-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Avoid deepcopy in pulse parser #9063
Conversation
This deepcopy is not strictly necessary; since we already own the nodes we're modifying, we only need to shallow copy nodes that we want to replace. In practice, the deep copy seems to cause difficult-to-resolve reference cycles within Jupyter that can get the parser.
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but I'm going to hold off on tagging this as automerge
until @nkanazawa1989 can take a look because he's the most familiar with this code.
Pull Request Test Coverage Report for Build 3384677280
💛 - Coveralls |
I wrote this almost 3 years ago but I don't quite remember why I needed the deep copy of the AST node here. This copy occurs to support parameter partial binding, so I believe this change should be okey as long as unittest for partial bindings works normally. Probably we can remove copy as well. Anyways partial binding will never occur in the current use case, and indeed this parser is sort of overengineering. Please go ahead with merging. |
This deepcopy is not strictly necessary; since we already own the nodes we're modifying, we only need to shallow copy nodes that we want to replace. In practice, the deep copy seems to cause difficult-to-resolve reference cycles within Jupyter that can get the parser. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 45f7836)
This deepcopy is not strictly necessary; since we already own the nodes we're modifying, we only need to shallow copy nodes that we want to replace. In practice, the deep copy seems to cause difficult-to-resolve reference cycles within Jupyter that can get the parser. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 45f7836) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
This deepcopy is not strictly necessary; since we already own the nodes we're modifying, we only need to shallow copy nodes that we want to replace. In practice, the deep copy seems to cause difficult-to-resolve reference cycles within Jupyter that can get the parser.
Details and comments
Fix #8783.
I was never able to use any of the cells given in that issue to reliably reproduce the issue, but I did seem to hit a similar issue when trying to transpile for an internal backend. This just ensures that all nodes that need to be mutated during the expression visit are only shallow copied, avoiding the deep-copy problem. I don't understand why that caused particular issues, though.
@nkanazawa1989: perhaps you can check me here, but I believe this should be fine and safe.