DYN-1662: As a dynamo dev, I want to serialize what packages are needed in a graph #9723
Conversation
|
@gregmarr PTAL this PR impacts the schema. |
|
Are the three levels of nesting here ( |
|
@gregmarr Yes, part of the idea here is to have a general way for extensions to serialize data to the .dyn file. In the future, this could include other internally developed extensions, and 3rd party extensions. These would all be siblings within As far as the schema inside of |
|
"part of the idea here is to have a general way for extensions to serialize data to the .dyn file. In the future, this could include other internally developed extensions, and 3rd party extensions. These would all be siblings within Extensions." We already explicitly allow additional data under the As this is data about which packages are needed to run the graph, this should be top-level data, as a sibling to the existing Something like this: |
|
@gregmarr thanks for the feedback. Makes sense. I’ll adopt the schema you’ve described here. |
|
Another thing that it would be good to add to a |
|
@scottmitchell what was the original idea of letting extensions to serialize their data as JSON? Are there cases where the graph (nodes) could have a dependency on extensions (other than packages)? |
|
@aparajit-pratap I'm not sure that there are cases where the graphs depend on an extension (although that's an interesting question). The idea here was just to explore the possibility of allowing extension developers to serialize data to the dyn—just a new, potentially useful api. |
…ame CollectingPackageDependencies -> CollectingLocalPackages.
| @@ -436,6 +442,23 @@ protected virtual void OnSaving(XmlDocument obj) | |||
| if (handler != null) handler(obj); | |||
| } | |||
|
|
|||
|
|
|||
| /// <summary> | |||
| /// Event that is fired when the workspace is collecting top level assemblies referenced by nodes | |||
There was a problem hiding this comment.
Is it true that only one workspace at a time will ever handle these events and only one extension will ever invoke them?
If those invariants needs to hold for this to make sense - then I think we should watch for those and log or throw if either of those are not true.
IE - check the invocationList and make sure it's only one etc. - At a minimum - add a comment.
|
@scottmitchell this is looking solid - I am still a bit concerned about serialization performance especially during backup save because the default backup time is pretty low at 60 seconds. I think we need to test this with a large graph - it might make sense to add serialization to the performance bench tool in the future - not sure that needs to happen here - but at the minimum we need to know the impact for a large graph with a large set of packages. (this can be done with a single unit test, profiling manually etc) |
|
@mjkkirschner I did a couple performance tests with and without package dependency collection/serialization. Looks like it makes serialization ~10% slower: Model.ToJson() time in ms
|
|
LGTM |
| @@ -800,6 +800,10 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s | |||
| writer.WriteEndArray(); | |||
| writer.WriteEndObject(); | |||
| } | |||
| else | |||
| { | |||
| logger.LogWarning("Unnsuccessful attempt to serialize a PackageDependencyInfo object.", Logging.WarningLevel.Moderate); | |||
There was a problem hiding this comment.
Does this need to be localizable?
There was a problem hiding this comment.
Good point, we should escape the localization of it
scottmitchell
changed the title
…ages are needed in a graph (#9723) * Add Extensions block to json * Serialize package manager data to new Extensions block * Added IPackage interface * Added IPackage interface * Added PackageDependencies property to workspace model * Added comments * Unsubscribe from workspace on dispose * Simplify CollectingAssembliesUsed * assemblyPackageDict uses AssemblyName objects instead of strings. Rename CollectingPackageDependencies -> CollectingLocalPackages. * NodeLibraries should be internal, not private * assemblyPackageDict uses AssemblyName.FullName * Use set for packageDependencies to avoid duplicates * Add some comments to assembly collection * AssemblyNames summary * Adds PackageDependency serialization support for custom nodes from packages * Rename GetCustomNodesPackagesFromGuids * Rename GetCustomNodesPackagesFromGuids * Add new PackageInfo class to replace IPackage for serialization * Remove IPackage * Move assembly name matching logic to PackageManager in order to remove AssemblyNames property from PackageInfo * Remove unnecessary * Fix AssemblyNames error * Null check for PackageInfo converter * Add obsolete attribute to events that should only be used by package manager * Use InternalsVisibleTo to ensure only PM extension can subscribe to package dependency events * Serialize IDs of nodes that are dependent on each package * Update package manager dictionary logic for package load and remove events * Fix Guid serialization * Add comment to InternalsVisibleTo * Name changes to make referencedAssembly use more clear * Add comments to Assembly collection and make more efficient * Comment added * Remove unnecessary ref * Removed Unecessary ref * Comment fixes * Rename PackageInfo -> PackageDependencyInfo * Use Version type for PackageDependencyInfo Version property * Rename Dependents -> Nodes * Move GetAssembliesReferencedByNodes * Added PackageDependency deserialization * Simplify if statement * Improve PackageDependencies update * PackageDependency deserialization null check * one line code improvements * IsNodeLibrary check * Remove unnecessary Any check * Add unsuccessful PackageDependencyInfo serialization log * Log message when node libraries or custom nodes are loaded by multiple packages * Improved RemovedNodes handling * Simplify descriptor lookup * Update comments * Store list of packages per assembly * Store list of packages per custom node id * Unsubscribe CurrentWorkspaceChanged * lock packageDependencies * Add NodePackageDictionary and CustomNodePackageDictionary null check * Move assembly collection to WorkspaceModel because LibraryServices no longer required * Remove white space * Add "PackageDependencies" item to test .dyn * Remove PackageDependencyInfo FullName property * Throw exception for illegal subscribers to PackageDependency events
Purpose
This PR adds
PackageDependenciesas a top level item in the .dyn file. Now, when a WorkspaceModel is serialized, it will collect locally installed package data from the package manager, and then serialize the data of those packages that are used in the graph. It currently looks like this:TODO:
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner @QilongTang
FYIs
@varvaratou