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

Add Python dependency info to packages and display it in DPM UI #11731

Merged
merged 9 commits into from
Jun 16, 2021

Conversation

zeusongit
Copy link
Contributor

Purpose

https://jira.autodesk.com/browse/DYN-3643

  • Adds a method to scan incoming packages for python nodes and dependent packages
  • Adds python engine dependency to package metadata.
  • Adds UI component to display dependencies by version
  • Updated UI to display latest version's dependencies

PS: One time process to scan and update all existing packages is under development.

SS:
Screen Shot 2021-06-03 at 12 38 40 AM

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

Reviewers

@DynamoDS/dynamo

@zeusongit zeusongit added the DNM Do not merge. For PRs. label Jun 3, 2021
@@ -1255,6 +1255,10 @@
<Project>{6e0a079e-85f1-45a1-ad5b-9855e4344809}</Project>
<Name>Units</Name>
</ProjectReference>
<ProjectReference Include="..\Libraries\PythonNodeModels\PythonNodeModels.csproj">
<Project>{8872CA17-C10D-43B9-8393-5C5A57065EB0}</Project>
<Name>PythonNodeModels</Name>
Copy link
Member

Choose a reason for hiding this comment

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

While I don't think this causes a circular dependency, it is a bit odd to have DynamoCoreWPF depend on a node library.

If we could figure out a way to solve this with reflection or some other mechanism - for example an interface in DynamoServiecs that might better decouple the core from our libraries and make it clear what the required interfaces actually are.

Copy link
Member

@mjkkirschner mjkkirschner Jun 4, 2021

Choose a reason for hiding this comment

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

On the other hand, one could make the argument that this is now a core requirement for a UI feature and as such we need to have this reference.

You may want to spend more time generalizing this if we still intend to allow for fully dynamic python engine loading in the future (theres a bunch of places that would need to be improved, and this is just one more) - if not, then maybe defining those interfaces is not worth it at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mjkkirschner I feel the problem can be traced back to why package manager UI is part of DynamoCoreWpf. To me it seems PM should be more a view extension but not the current way that UI in CoreWpf but data model as extension.. On that note, if the original design is that PM should be part of Core then I feel this dependency is accurate.. Or we could move Python loading to DynamoCore but that also seems not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

@QilongTang - there is actually a PackageManagerUIViewExtension - we just never finished moving the UI out of core and into the view extension :) - more tech debt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.. @mjkkirschner Then are you fine of this temp dependency and we shall get to it. Our team have Package Manager UI refresh this quarter and it seems the tech debt can be address in that epic?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good - maybe we just make a jira task for this (python references) and another for the packageManager UI refactor tech debt to track both of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do @mjkkirschner

Copy link
Contributor

Choose a reason for hiding this comment

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

@zeusongit zeusongit marked this pull request as ready for review June 7, 2021 16:41
@zeusongit zeusongit requested a review from QilongTang June 7, 2021 16:41
@zeusongit zeusongit removed the DNM Do not merge. For PRs. label Jun 7, 2021
@mjkkirschner
Copy link
Member

@zeusongit @QilongTang - I see the tests are failing - my guess is because some python assemblies are being copied to multiple folders now because of the new references.

mark it private false or add a cleanup target to one of the scripts we have to delete those binaries.

@zeusongit zeusongit merged commit ee4113d into DynamoDS:master Jun 16, 2021
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