| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-2763 Add options to change Python Engine in multiple python nodes #15475
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
Conversation
|
In my opinion it would be ok to wait on dyf support |
zeusongit
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-2763
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.
LGTM with some comments
|
Thanks, waiting for PR checks to finish |
|
Added DYF support. |
| { | ||
| if (!Workspaces.OfType<CustomNodeWorkspaceModel>().Contains(customNodeWorkspace)) | ||
| { | ||
| AddWorkspace(customNodeWorkspace); |
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.
After the custom node workspace loaded silently, we should also remove it somehow after Python engine switched?
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 thought about that as well, but the workspaces should/are marked dirty IMO, so user will know which custom nodes were updated. One scenario where this would be bad is maybe if a user has too many custom nodes?
Also saving the custom nodes for them and then closing the workspace seemed a bit hacky, would you like that instead?
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 would prefer the current option then, maybe when customer has too many dyfs in the canvas, then this could potentially be a problem
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.
Yes, we could consult customers for feedback and then later update this functionality.
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.
The silent loading seems safe to me, I can't think of any side effects for now other than maybe more memory used. LGTM with one question about workspace unloading
Purpose
This PR adds 2 additional ways to update Python Engine of multiple python nodes
Can Handle DYF/Custom nodes
TODO:
Add support for DYF'sDeclarations
Check these if you believe they are true
*.resxfilesRelease Notes
Reviewers
@DynamoDS/dynamo