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-6769 improving dynamo load graph ii 2 #15158

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

Modifying the DefaultNodeAutocompleteCandidates functionality so is only executed once.
Instead of using a static Dictionary now I will be using a normal instance that will be initialized in DynamoViewModel and pass it to NodeAutoCompleteSearchViewModel, in this case we are also confirming that only be executed once.

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

Modifying the DefaultNodeAutocompleteCandidates functionality so is only executed once.

Reviewers

@QilongTang

FYIs

RobertGlobant20 and others added 3 commits April 15, 2024 12:26
* DYN-6769 Improving Dynamo Load Graph

With this change only will be executing the DefaultAutocompleteCandidates functionality only once at Dynamo startup and when a graph is loaded won't be executed anymore.

* DYN-6769 Improving Dynamo Load Graph

With this change only will be executing the DefaultAutocompleteCandidates functionality only once at Dynamo startup and when a graph is loaded won't be executed anymore.
Instead of using a static Dictionary now I will be using a normal instance that will be initialized in DynamoViewModel and pass it to NodeAutoCompleteSearchViewModel, in this case we are also confirming that only be executed once.
Disabling the DefaultNodeAutocomplete functionality when Dynamo is in ServiceMode.
@RobertGlobant20
Copy link
Contributor Author

GIF showing that the Node Autocomplete functionality is working.
bNdcaDrzhy

@RobertGlobant20
Copy link
Contributor Author

Also I've run this changes in the DYN-DevCI_Self_Service job and all tests are passing.

Copy link

github-actions bot commented Apr 18, 2024

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@QilongTang QilongTang added this to the 3.2 milestone Apr 18, 2024
var queries = new List<string>() { "String", "Number Slider", "Integer Slider", "Number", "Boolean", "Watch", "Watch 3D", "Python Script" };
foreach (var query in queries)
{
var foundNode = tempSearchViewModel.Search(query).Where(n => n.Name.Equals(query)).FirstOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to call the search API from tempSearchViewModel, just curious if this object intialization can be skipped?

Copy link
Contributor Author

@RobertGlobant20 RobertGlobant20 Apr 19, 2024

Choose a reason for hiding this comment

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

Well, my idea was to get the nodes for DefaultNodeAutocomplete at DynamoViewModel level and then pass them to the NodeAutoCompleteSearchViewModel instance (so in this way the search will be executed once per each node).

If you remember at Dynamo startup we create one DynamoViewModel but inside this we are creating two WorkpaceViewModel instances and inside each instance we are creating the NodeAutoCompleteSearchViewModel instance and the SearchViewModel instance, so the SearchViewModel instance is at WorkpaceViewModel level that's why I had to create the tempSearchViewModel instance (SearchViewModel) at DynamoViewModel level (we don't have that instance at that level) otherwise I won't be able to execute the Search for each node.

Let me know if it sounds clear to you or otherwise I can make a diagram showing the details.
Thanks

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 one question

@RobertGlobant20
Copy link
Contributor Author

By the way I can see the Dynamo SelfServe check failed due to the next test.
Dynamo.Tests.WorkspaceSaving.RemovePIIDataFromWorkspace

But I executed the same branch in the DYN-DevCI_Self_Service job two times and both passed.

@reddyashish
Copy link
Contributor

@RobertGlobant20 can you mark that test as failure on master branch. It has been marked on 3.1 branch but forgot to make that change on master.

Marking the test RemovePIIDataFromWorkspace as Failure
@RobertGlobant20
Copy link
Contributor Author

@RobertGlobant20 can you mark that test as failure on master branch. It has been marked on 3.1 branch but forgot to make that change on master.

done in the next commit: 4d348f3

@reddyashish reddyashish merged commit 50627dd into DynamoDS:master Apr 24, 2024
23 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