Skip to content

Conversation

@chubakueno
Copy link
Contributor

@chubakueno chubakueno commented Feb 28, 2025

Purpose

The dependency graph currently accumulates old versios of the nodes (modified and deleted) indefinitely while the Dynamo workspace is running and being edited. This PR aims to clean up old node versions to keep the quadratic complexity of BuildGraphNodeDependencies controlled.

Related performance spike: #15864

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

Clean up dependency graph to improve performance and prevent progressive Dynamo slowing down.

Reviewers

@QilongTang

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@chubakueno chubakueno added the WIP label Feb 28, 2025
@QilongTang QilongTang added this to the 3.5 milestone Feb 28, 2025
return;
}

for (int i = 0; i < graphNodesInScope.Count; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you add comments to this block, seems pretty important for others to understand

Copy link
Contributor Author

@chubakueno chubakueno Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ChildrenNodes and ParentNodes are being rebuilt every time, but before this PR they were accumulating more dependencies each time BuildGraphNodeDependencies was called. So this is more of a bug correction.

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.

Looks solid to me, waiting for @aparajit-pratap @saintentropy to comment

@QilongTang
Copy link
Contributor

QilongTang commented Feb 28, 2025

The Jira ticket check fails when Jira issue is not part of the PR title, I will do an example fix for you. When Jira issue correctly referenced in PR title, this PR will be linked to Jira task correctly for future reference

@QilongTang QilongTang changed the title cleanup dependency graph from old nodes DYN-8358 cleanup dependency graph from old nodes Feb 28, 2025
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-8358

@mjkkirschner
Copy link
Member

seems like some real test failures.

@QilongTang
Copy link
Contributor

seems like some real test failures.

yup, @chubakueno are you able to access PR check test failures?

@QilongTang
Copy link
Contributor

Do you have the reported failure test Dynamo.Tests.Models.NodeModelWarningsTest.CombinedBuildAndRuntimeWarnings passing locally?

@BogdanZavu
Copy link
Contributor

Looks solid to me, waiting for @aparajit-pratap @saintentropy to comment

Same here. The dependency graph still grows but at a much slower rate.
We would like some blessings from the people who spent more time in this area.

@QilongTang
Copy link
Contributor

@sm6srw We see EngOps Build check failing here, expected?

@QilongTang
Copy link
Contributor

QilongTang commented Mar 7, 2025

@chubakueno Looking at this again, my wild guess of the PR check failure is that this fork is not up-to-date with master. I decide to trust that you had all the checks passing earlier this week. I am leaning towards that we merge and test the mock cluster placement -> undo repeated to validate it, as you already did locally. @BogdanZavu What do you think?

@BogdanZavu
Copy link
Contributor

@chubakueno Looking at this again, my wild guess of the PR check failure is that this fork is not up-to-date with master. I decide to trust that you had all the checks passing earlier this week. I am leaning towards that we merge and test the mock cluster placement -> undo repeated to validate it, as you already did locally. @BogdanZavu What do you think?

Sounds good to me. Nice work!

@QilongTang QilongTang merged commit 3cb925b into master Mar 10, 2025
22 of 24 checks passed
@QilongTang QilongTang deleted the improve_dependency_graph_performance branch March 10, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants