Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Dag.ch_func and signature comparison #45

Closed
thorwhalen opened this issue Jan 3, 2023 · 1 comment
Closed

Dag.ch_func and signature comparison #45

thorwhalen opened this issue Jan 3, 2023 · 1 comment
Assignees

Comments

@thorwhalen
Copy link
Member

thorwhalen commented Jan 3, 2023

In solving DAG.ch_func should be more robust issue with this commit, we broke CI:
When making ch_funcs less permissive, test_dag_operations stopped working because the injected functions had defaults whereas the (code_to_dag-made) dag template did not.
Removed the defaults of injected functions, but am writing an issue about this since we really need more flexible signature comparisons.

In the particular instance of Dag.ch_func, we really want a user to specify a minimum when they first make the template, and then be able to assemble the functions they need and inject them in the dag. Aspects like defaults shouldn't show up there, so injecting a function that has a default shouldn't be a problem. Here, perhaps the validation/comparison rule could be "it's okay to add a default (i.e. inject a func with a default in a FuncNode whose func didn't have a default), but not to change it?

See the Signature binary function (e.g. comparison) design issue for more on this.

Current state of affairs

    >>> from i2 import Sig
    >>>
    >>> dag = DAG([
    ...     FuncNode(lambda a, b: a + b, name='f'),
    ...     FuncNode(lambda y=1, z=2: y * z, name='g', bind={'z': 'f'})
    ... ])
    >>>
    >>> Sig(dag)
    <Sig (a, b, f=2, y=1)>
    >>>

If you replace by a different function with exactly the same signature,
all goes well:

    >>> dag.ch_funcs(g=lambda y=1, z=2: y / z)
    DAG(func_nodes=[FuncNode(a,b -> f -> _f), FuncNode(z=_f,y -> g -> _g)], name=None)

But if you change the signature, even slightly you get an error.

Here we didn't include the defaults:

    >>> dag.ch_funcs(g=lambda y, z: y / z)  # doctest: +NORMALIZE_WHITESPACE +ELLIPSIS
    Traceback (most recent call last):
      ...
    ValueError: You can only change the func of a FuncNode with a another func if the signatures match.
      ...

Here we include defaults, but z's is different:

    >>> dag.ch_funcs(g=lambda y=1, z=200: y / z)  # doctest: +NORMALIZE_WHITESPACE +ELLIPSIS
    Traceback (most recent call last):
      ...
    ValueError: You can only change the func of a FuncNode with a another func if the signatures match.
      ...

Here the defaults are exactly the same, but the order of parameters is
different:

    >>> dag.ch_funcs(g=lambda z=2, y=1: y / z)  # doctest: +NORMALIZE_WHITESPACE +ELLIPSIS
    Traceback (most recent call last):
      ...
    ValueError: You can only change the func of a FuncNode with a another func if the signatures match.
      ...

Such sensitivity, though desirable as a default since it makes things robust out of the box, in many contexts it is quite annoying.

Therefore we need to allow the user to specify the signature comparison function.

thorwhalen added a commit that referenced this issue Jan 3, 2023
When making ch_funcs less permissive, test_dag_operations stopped working because the injected functions had defaults whereas the (code_to_dag made) dag template did not.
Removed the defaults of injected functions, but wrote issue (#45) about this since we really need more flexible signature comparisons.
@thorwhalen
Copy link
Member Author

Closed with a451ef3

But more tests to come from @sylvainbonnot .

@sync-by-unito sync-by-unito bot reopened this Jan 20, 2023
@i2mint i2mint locked and limited conversation to collaborators Apr 12, 2023
@thorwhalen thorwhalen converted this issue into discussion #52 Apr 12, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

3 participants