-
Notifications
You must be signed in to change notification settings - Fork 114
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
[review] Dask performance tuning + Dask plugin + Smart Cache handling + Composite Node performance tuning + Template Node #123
Conversation
I the latest change, I added |
This should be my last round of change. The composite node only build the graph once in the
|
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.
I'm still reviewing and testing. Left two comments so far.
gQuant/plugins/dask_plugin/greenflow_dask_plugin/simpleParallelNode.py
Outdated
Show resolved
Hide resolved
gQuant/plugins/dask_plugin/greenflow_dask_plugin/simpleParallelNode.py
Outdated
Show resolved
Hide resolved
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.
I left a few comments. The logic in the SimpleNodeMixin
seems to me like it really should be part of NodeTaskGraphMixin
. Let me know what you think.
Can you explain a bit more regarding the SimpleNodeMixin
, especially its update
, ports_setup
and meta_setup
methods, if you disagree with me regarding that belonging in the NodeTaskGraphMixin
? I think all nodes should have this logic injected when they are within a task graph. It shouldn't be necessary to inherit explicitly from any mixin for a Node's implementation.
Refactored meta/ports resolve and extended logic.
Refactor SimpleNodeMixin.
I made a few changes to make the template state private. I checked all the notebooks and unit tests. |
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.
Looks good. Everything works. I'll merge this.
In this PR, I made following changes:
I tested all the notebooks.