Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Handling of Process Parameters and Defaults #171

Open
U8NWXD opened this issue Mar 18, 2022 · 2 comments
Open

Improve Handling of Process Parameters and Defaults #171

U8NWXD opened this issue Mar 18, 2022 · 2 comments

Comments

@U8NWXD
Copy link
Member

U8NWXD commented Mar 18, 2022

Status Quo

Current Approach

Currently, processes have a defaults class variable that specifies the parameters they expect. Then when calling a process, you can provide parameters in a dictionary to the parameters argument to override those defaults. For example:

class MyProcess(Process):

    defaults = {
        'flag': True,
    }

proc = MyProcess({'flag': False})

Problems with Current Approach

Our current approach for handling process parameters and defaults has a number of issues:

  1. Parameters do not have types, which prevents type checking from catching typing errors. For example, you could write MyProcess({'flag': 'test'}) without raising any type errors.
  2. Similarly, we don't actually enforce the convention that all accepted parameters be present in defaults. For example, MyProcess could have a next_update that uses self.parameters['flag2'] even though MyProcess.defaults does not contain flag2.
  3. Other Python tools don't understand our way of specifying parameters. This means that e.g. PyCharm won't suggest parameters when you initialize a process.
  4. Our approach is not the standard Python way of specifying parameters, which is confusing.
  5. defaults are not handled correctly when subclassing processes. For example, consider the process class MyProcess2(MyProcess) with a constructor that calls MyProcess.__init__(parameters). Inside MyProcess.__init__, self.defaults refers to MyProcess2.defaults, not MyProcess.defaults like you'd expect.

Functionality to Preserve

  1. Backwards compatibility. We don't want to have to rewrite all our libraries.
  2. Ability to save off parameters for serialization.

Possible Alternative

All the problems would be solved by using normal Python function arguments for process parameters. Here's how we could preserve the desired functionality:

  1. Not sure how we can do this yet.

  2. I think we can use inspect and locals() to do this. We would, in Process.__init__(), first use self.__class__ to get the subclass. Then we could use code like this to get the parameters passed to the subclass constructor:

    >>> def foobar(foo, bar, baz):
    ...     sig, foobar_locals = inspect.signature(foobar), locals()
    ...     return [foobar_locals[param.name] for param in sig.parameters.values()]
    ...
    >>> foobar(1, 2, 3)
    [1, 2, 3]

    Source: https://stackoverflow.com/a/10724602

    Note that the locals() call would have to happen in the subclass, and the result would need to be passed to the superclass constructor.

@eagmon
Copy link
Member

eagmon commented Mar 28, 2022

Some questions about the alternative @U8NWXD:

  1. Is the only required backwards compatibility to support a self.defaults dict and a parameters dict being passed to the Process superclass with super().__init__(parameters)? Anything else?
  2. Is the only purpose of having the Process superclass access the subclass locals to save these parameters for serialization?

@U8NWXD
Copy link
Member Author

U8NWXD commented Mar 29, 2022

Regarding #171 (comment):

  1. I think we also need to handle of special parameters like time_step and _original_parameters, and we need to be able to access parameters through self.parameters.
  2. The Process superclass also performs other operations like handling those special parameters, schema overrides, and merging the provided parameters with the defaults. More broadly though, the Process class adds a layer of indirection into which we can insert code to make changes based on what the user does. For example, we use get_schema() to apply schema overrides on top of what the user specifies in ports_schema().

@eagmon eagmon mentioned this issue Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants