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

Implement a non-in-place version of FunctionGraph.change_input #362

Open
brandonwillard opened this issue Apr 5, 2021 · 1 comment · May be fixed by #666
Open

Implement a non-in-place version of FunctionGraph.change_input #362

brandonwillard opened this issue Apr 5, 2021 · 1 comment · May be fixed by #666
Labels
enhancement New feature or request graph rewriting help wanted Extra attention is needed important

Comments

@brandonwillard
Copy link
Member

brandonwillard commented Apr 5, 2021

FunctionGraph.change_input changes an Apply node's inputs in-place, but we could just as easily clone and replace the affected Apply node(s).

FYI: FunctionGraph.change_input is what drives FunctionGraph.replace, and the latter is what—in turn—drives all the graph rewriting that occurs with GlobalOptimizer, LocalOptimizer, etc.

This approach would prevent the need to clone graphs before creating a FunctionGraph, by—instead—cloning "on-demand" and only when and where it's actually required. It would also be a convenient way to preserve the identity of unrelated nodes and reduce the need for a lot of clone-to-original-variable mapping logic.

I started an implementation here (and a test here), but that simple approach won't really work.

Also, we would need to provide a new callback for this type of update, and, if we make this approach the default, we would need to update the Features that use the old "on_change_input" callbacks so that they use this new one.

@brandonwillard brandonwillard added enhancement New feature or request help wanted Extra attention is needed important graph rewriting labels Apr 5, 2021
@brandonwillard brandonwillard pinned this issue May 24, 2021
@brandonwillard
Copy link
Member Author

This would help with one of the problems in #431, since this approach to graph mutation doesn't avoid calling Apply.__init__.

@brandonwillard brandonwillard changed the title Provide a non-in-place version of FunctionGraph.change_input Implement a non-in-place version of FunctionGraph.change_input May 24, 2021
@kc611 kc611 linked a pull request Nov 14, 2021 that will close this issue
7 tasks
@twiecki twiecki unpinned this issue Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request graph rewriting help wanted Extra attention is needed important
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant