Efficiently solve dependency metadata on install #10544

mmisol · 2020-04-06T14:28:25Z

Purpose

When resolving dependencies from the workspace dependency extension,
the entire metadata of dependent packages was requested. However, only
information about the version was truly needed, and for packages with a
lot of versions this was causing latency issues.

Now only the package version of dependencies is obtained. As a result,
several code changes were needed, as part of the code path is shared
with the standard installation of packages through search, but with
different data requirements in each case. Several properties and
functions no longer used were marked as 'Obsolete'.

Added some basic unit tests for the new functions in package manager
client.

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.

Reviewers

@mjkkirschner @zeusongit

FYIs

@reddyashish

…eader_package_install

mjkkirschner · 2020-04-06T14:56:24Z

src/DynamoCoreWpf/Properties/Resources.en-US.resx

@@ -2233,6 +2229,10 @@ Uninstall the following packages: {0}?</value>
  <data name="DynamoViewSettingsMenuShowCodeBlockNodeLineNumber" xml:space="preserve">
    <value>Show CodeBlockNode Line Numbers</value>
  </data>
+  <data name="MessageFailedToDownloadPackageVersion" xml:space="preserve">
+    <value>Failed to download version {0} of package with id: {1}.  Please try again and report the package if you continue to have problems.</value>
+    <comment>Message box content. {0} = 1.2.3, {1} = 57d576e8f615e7725800001d</comment>


are these default values?

No, just examples of what kind of text is expected in the placeholders. I thought that's a good way of communicating that to whoever does the text translations.

mjkkirschner · 2020-04-06T16:31:44Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

@@ -701,7 +704,7 @@ internal void DownloadAndInstall(PackageDownloadHandle packageDownloadHandle, st
                        Package dynPkg;

                        var pmExtension = DynamoViewModel.Model.GetPackageManagerExtension();
-                        var firstOrDefault = pmExtension.PackageLoader.LocalPackages.FirstOrDefault(pkg => pkg.Name == packageDownloadHandle.Name);
+                        var firstOrDefault = pmExtension.PackageLoader.LocalPackages.FirstOrDefault(pkg => pkg.ID == packageDownloadHandle.Id);


hmm, do you think this could change any existing behavior?

I don't think so. I tested installing packages from both the search and the workspace dependency extension to be sure.

mjkkirschner · 2020-04-06T16:32:18Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

@@ -721,18 +724,13 @@ internal void DownloadAndInstall(PackageDownloadHandle packageDownloadHandle, st

                        if (packageDownloadHandle.Extract(DynamoViewModel.Model, downloadPath, out dynPkg))
                        {
-                            var p = Package.FromDirectory(dynPkg.RootDirectory, DynamoViewModel.Model.Logger);


why is this no longer required?

This was already being done inside packageDownloadHandle.Extract, but only for the workspace dependency extension code path. I unified both code paths and now it's always done inside Extract, which returns a boolean to report success.

mjkkirschner · 2020-04-06T16:35:43Z

@mmisol looks good to me!

gregmarr · 2020-04-06T16:51:11Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

-                        x => x.Item2.contents.Contains(PackageManagerClient.PackageContainsPythonScriptsConstant));
+                var containsBinariesOrPythonScripts = dependencyVersionHeaders.Any(x => 
+                    x.contents.Contains(PackageManagerClient.PackageContainsBinariesConstant) ||
+                    x.contains_binaries ||


We would get some minor performance gains by moving the contains_binaries test before the Contains() check.

QilongTang · 2020-04-06T17:27:19Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

            }
        }

        /// <summary>
        ///     Returns a newline delimited string representing the package name and version of the argument
        /// </summary>
+        [Obsolete("No longer used. Remove in 3.0.")]


@mjkkirschner I thought we made it clear somewhere that these APIs are not part of Dynamo SDK? If not, let's do that..

@QilongTang we only made that statement in new extension binaries like WorkspaceReferencesExtension - this is a public method in DynamoCoreWPF - we can't remove this method now without some of the other work on reorganizing or componentizing the API we've been discussing.

Thank you for clarifying. I thought this is in WorkspaceReference project. My bad

QilongTang · 2020-04-06T17:28:28Z

src/DynamoPackages/PackageManagerClient.cs

@@ -122,6 +123,7 @@ internal PackageHeader GetPackageMaintainers(IPackageInfo packageInfo)
        /// <param name="header"></param>
        /// <param name="version"></param>
        /// <returns></returns>
+        [Obsolete("No longer used. Remove in 3.0.")]


@mmisol I believe Internal APIs can be removed anytime. Unless it is used in a lot of other projects?

mjkkirschner · 2020-04-06T18:29:25Z

src/DynamoPackages/PackageDownloadHandle.cs

        public Greg.Responses.PackageHeader Header { get; private set; }
        private string _name;
        public string Name { get { return Header != null ? Header.name : _name; } set { _name = value; } }

+        private string _id;
+        public string Id { get { return Header != null ? Header._id : _id; } set { _id = value; } }


for public properties please add /// summaries - also, should this really be public? Do we intend for others extension authors / dynamo integrators to use this?

This is used in DynamoCoreWpf so I'll add comments for it. I don't think someone might want to extend it in some way. This is used as a view model for the installed dependencies when installing a package from the search.

if it's only to be used by us - maybe we can make it internal and use the internalsVisibleTo attribute, my feeling is that we should be much more deliberate about what constitutes our public api. What do you think?

My opinion is that using InternalsVisibleTo is more of a hack than a good practice. Making something public shouldn't make it "written in stone". I think we should identify exactly what we want our extension points to be instead, and only make promises of backwards compatibility for those.

mmisol added 8 commits March 27, 2020 12:14

wip: local setup, name not shown

9bb662b

Fix name not appearing in PM install

2e79cc0

WIP

8708787

Undo local test changes

155f3b5

More undoing test changes

d1fbf58

Merge branch 'master', remote-tracking branch 'origin' into version_h…

07d3d68

…eader_package_install

Undo commit that should not be merged

d2d473a

Add unit tests

4876b6a

mmisol requested review from reddyashish, mjkkirschner and zeusongit April 6, 2020 14:28

mjkkirschner reviewed Apr 6, 2020

View reviewed changes

mjkkirschner approved these changes Apr 6, 2020

View reviewed changes

gregmarr reviewed Apr 6, 2020

View reviewed changes

QilongTang reviewed Apr 6, 2020

View reviewed changes

mjkkirschner reviewed Apr 6, 2020

View reviewed changes

mmisol added 2 commits April 6, 2020 14:33

Delete unused/obsolete, optimize binary check

250e112

Comment class and public members

7612300

zeusongit approved these changes Apr 6, 2020

View reviewed changes

mmisol merged commit 6c994f6 into DynamoDS:master Apr 6, 2020

aparajit-pratap mentioned this pull request Apr 11, 2020

[DYN-2609] Fix crash when installing empty package #10561

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Efficiently solve dependency metadata on install #10544

Efficiently solve dependency metadata on install #10544

mmisol commented Apr 6, 2020 •

edited

Loading

mjkkirschner Apr 6, 2020

mmisol Apr 6, 2020

mjkkirschner Apr 6, 2020

mmisol Apr 6, 2020

mjkkirschner Apr 6, 2020

mmisol Apr 6, 2020

mjkkirschner commented Apr 6, 2020

gregmarr Apr 6, 2020

QilongTang Apr 6, 2020

QilongTang Apr 6, 2020

mjkkirschner Apr 6, 2020

QilongTang Apr 6, 2020

QilongTang Apr 6, 2020 •

edited

Loading

mjkkirschner Apr 6, 2020 •

edited

Loading

mmisol Apr 6, 2020

mjkkirschner Apr 6, 2020 •

edited

Loading

mmisol Apr 6, 2020 •

edited

Loading

Efficiently solve dependency metadata on install #10544

Efficiently solve dependency metadata on install #10544

Conversation

mmisol commented Apr 6, 2020 • edited Loading

Purpose

Declarations

Reviewers

FYIs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

mmisol Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

mmisol commented Apr 6, 2020 •

edited

Loading

QilongTang Apr 6, 2020 •

edited

Loading

mjkkirschner Apr 6, 2020 •

edited

Loading

mjkkirschner Apr 6, 2020 •

edited

Loading

mmisol Apr 6, 2020 •

edited

Loading