DYN-7535 Record python engine package information in graphs when using engines served from them#15535
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7535
UI Smoke TestsTest: success. 11 passed, 0 failed. |
| /// The method returns the assembly name from which the node originated. | ||
| /// </summary> | ||
| /// <returns>Assembly Name</returns> | ||
| internal virtual AssemblyName GetNameOfAssemblyReferencedByNode() |
There was a problem hiding this comment.
maybe a better name is GetOwningAssemblyNameOfNodeModel - referenced by node sounds like a dependency of the node to me anyway.
| AssemblyName assemblyName = null; | ||
|
|
||
| var descriptor = this.Controller.Definition; | ||
| if (descriptor.IsPackageMember) |
There was a problem hiding this comment.
I think this would be valid even if this was not a packaged node -right?
There was a problem hiding this comment.
Yes, it would be, but I think the intent is to return null for non-packaged nodes.
There was a problem hiding this comment.
then I think your summary/returns could be updated to reflect that / a test should be added encoding that behavior.
There was a problem hiding this comment.
Sure, just want to add that this was the default behavior, I did not add this code but moved it inside the base class. So whatever it was doing before will still be applied.
| { | ||
| var assemName = AssemblyName.GetAssemblyName(assem.Location); | ||
| OnMessageLogged(LogMessage.Info( | ||
| string.Format("{0} contains the python engine library {1}, which has already been loaded " + |
There was a problem hiding this comment.
nitpick IMO more readable to use string interpolation
https://stackoverflow.com/questions/33236592/multiline-c-sharp-interpolated-string-literal
|
nice, this seems much cleaner, and should be testable by creating a graph with a bunch of python nodes in it and checking the |
Sure, added a test |
|
One sporadic test failing, has been failing for most PRs |
test/pkgs/PythonEnginePackage/extra/DSPythonNet3Extension.deps.json
Outdated
Show resolved
Hide resolved
5 participants
Purpose
A leaner approach to record package dependency for a python engine, and trigger workspace dependency.
Opening a graph with PythonEngine not loaded in Dynamo:
Opening a graph with PythonEngine loaded in Dynamo, but with a version mis-match:
DynamoSandbox_q0lBECS4VN.mp4
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Reviewers
@DynamoDS/dynamo
FYIs
@pinzart90