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

DYN-2763 Add options to change Python Engine in multiple python nodes #15475

Merged
merged 13 commits into from
Sep 11, 2024

Conversation

zeusongit
Copy link
Contributor

@zeusongit zeusongit commented Sep 10, 2024

Purpose

This PR adds 2 additional ways to update Python Engine of multiple python nodes

  • Via workspace context menu - will update the selected python nodes
  • Via Main Edit menu option - will update all python nodes present in the current workspace

DynamoSandbox_pnk6wsXOCj

Can Handle DYF/Custom nodes

DynamoSandbox_nQfQQsl5N6

TODO:

  • Add support for DYF's
  • Tests

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

  • Add options to change Python Engine in multiple python nodes

Reviewers

@DynamoDS/dynamo

@QilongTang QilongTang added this to the 3.3 milestone Sep 10, 2024
@twastvedt
Copy link
Contributor

In my opinion it would be ok to wait on dyf support

@zeusongit zeusongit changed the title Add options to change Python Engine in multiple python nodes DYN-2763 Add options to change Python Engine in multiple python nodes Sep 10, 2024
Copy link

@github-actions github-actions bot left a 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

Copy link
Contributor

@QilongTang QilongTang left a 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

@QilongTang
Copy link
Contributor

Thanks, waiting for PR checks to finish

@zeusongit
Copy link
Contributor Author

zeusongit commented Sep 10, 2024

Added DYF support.
The custom node workspaces will be open in the background and marked dirty.

{
if (!Workspaces.OfType<CustomNodeWorkspaceModel>().Contains(customNodeWorkspace))
{
AddWorkspace(customNodeWorkspace);
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@QilongTang QilongTang left a 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

@QilongTang QilongTang merged commit 6fd8000 into DynamoDS:master Sep 11, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants