Switch to use sync version of CPython setup/installation method #11428

QilongTang · 2021-01-26T15:50:15Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Regarding https://jira.autodesk.com/browse/REVIT-173181.

Switch to use sync version of CPython setup/installation method as suggested by @mmisol .

If team is OK with this, let's ship the hotfix this way. Notice the little hang only happens first time user trying to evaluate a CPython node

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

FYIs

@mmisol @mjkkirschner @saintentropy

mmisol · 2021-01-26T16:15:39Z

src/Libraries/DSCPython/CPythonEvaluator.cs

+                Task.Run(() => {
+                   Python.Included.Installer.SetupPython().Wait();
+                   isPythonInstalled = true;
+               }).Wait();


Hey Aaron. I must admit when I heard about the hang I remember I also happened to experience it. Back then, what I did to get rid of it is to make SetupPython fully synchronous, which is actually a simple change in our Python.Included fork https://git.autodesk.com/Dynamo/pythonnet/pull/14/files#diff-4cc075ba367f8ff85f7aff06fd5c060aR30. That change didn't make it into master because it was part of the draft for Python module installation support, which is still a RFC.
IMO, making SetupPython synchronous is a simpler solution. Besides, since the unzip action is almost instantaneous and runs only once it doesn't make much sense for it to be async. That would mean that we would just call Python.Included.Installer.SetupPython(); here.
An alternative, if we don't want to change the existing SetupPython function, could be to create a synchronous variation named SetupPythonSync that basically uses the same code I linked.
What do you think about this @QilongTang @mjkkirschner ?

I like adding the new method!

what do you guys think about actually wrapping pythonIncluded in our own namespace? I guess I am concerned that if we need custom functionality that and if a regular build of Python.Included were loaded into Revit's process by another addin (pyrevit) we would hit a missing method exception.

I guess we could do that but not sure if now is a good time, given it would represent a breaking change. Python .NET could also potentially present clashes. Not sure if we have tested Revit + pyRevit + Dynamo, but we should probably do it to have an idea of how urgent this change should be. What do you think @mjkkirschner ?

@mmisol I'm fine waiting as well and we can discuss those changes together instead - but I am curious - in what sense is it a breaking change?

@mmisol I'm fine with that, I will make that change in this PR soon

Hi @mmisol You just reminded me, we tried that yesterday and found we need to wait for the unzip to finish before executing the graph. If we just call Python.Included.Installer.SetupPython(); the install code in our PythonNet is async so user will get this error

@mjkkirschner Changing the namespace of a public class is technically a breaking change. Probably no-one is using it directly except us, so it could be low risk.

@QilongTang We don't just need a change in Dynamo, but we also need to change Python.Included. If you check the link I shared earlier you'll see the SetupPython function was changed to being synchronous, though It's probably best if we add a new one named SetupPythonSync and leave SetupPython unchanged.

@mmisol Cool. Go ahead for that

mjkkirschner · 2021-01-26T21:31:26Z

This also needs to consume the new binary right? (I think manually copy to extern)

QilongTang · 2021-01-26T21:36:22Z

This also needs to consume the new binary right? (I think manually copy to extern)

Indeed, @mmisol helped #11429

mmisol · 2021-01-26T22:18:07Z

@QilongTang Could you search for SetupPython().Wait in the code base? I'm pretty sure this was being done someplace else. It would be nice to make the fix there too.

mmisol

Looks good to me. Thanks!

* Use task run to wrap the installation body * Update to use sync version of API * Additional place to fix

…) (#11435) * Switch to use sync version of CPython setup/installation method (#11428) * Use task run to wrap the installation body * Update to use sync version of API * Additional place to fix * Update Python.Included to PR 22 (#11429) Adds function SetupPythonSync to setup python synchronously Co-authored-by: Martin Misol Monzo <[email protected]>

Use task run to wrap the installation body

6449af0

mmisol reviewed Jan 26, 2021

View reviewed changes

Update to use sync version of API

2f67f4a

QilongTang changed the title ~~Use task run to wrap the CPython installation body~~ Switch to use sync version of CPython setup/installation method Jan 26, 2021

Merge branch 'master' into TestBranch

f1d7287

Additional place to fix

2c8a334

QilongTang marked this pull request as ready for review January 26, 2021 22:54

mmisol approved these changes Jan 27, 2021

View reviewed changes

QilongTang merged commit 969e347 into master Jan 27, 2021

QilongTang deleted the TestBranch branch January 27, 2021 16:04

QilongTang added a commit that referenced this pull request Jan 27, 2021

Switch to use sync version of CPython setup/installation method (#11428)

01881f4

* Use task run to wrap the installation body * Update to use sync version of API * Additional place to fix

QilongTang mentioned this pull request Jan 27, 2021

Switch to use sync version of CPython setup/installation method (#11428) #11435

Merged

8 tasks

QilongTang restored the TestBranch branch February 1, 2021 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to use sync version of CPython setup/installation method #11428

Switch to use sync version of CPython setup/installation method #11428

QilongTang commented Jan 26, 2021 •

edited

Loading

mmisol Jan 26, 2021

mjkkirschner Jan 26, 2021

mjkkirschner Jan 26, 2021 •

edited

Loading

mmisol Jan 26, 2021

mjkkirschner Jan 26, 2021

QilongTang Jan 26, 2021 •

edited

Loading

QilongTang Jan 26, 2021

mmisol Jan 26, 2021

QilongTang Jan 26, 2021

mjkkirschner commented Jan 26, 2021

QilongTang commented Jan 26, 2021 •

edited

Loading

mmisol commented Jan 26, 2021

mmisol left a comment

Switch to use sync version of CPython setup/installation method #11428

Switch to use sync version of CPython setup/installation method #11428

Conversation

QilongTang commented Jan 26, 2021 • edited Loading

Purpose

Declarations

Reviewers

FYIs

mmisol Jan 26, 2021

Choose a reason for hiding this comment

mjkkirschner Jan 26, 2021

Choose a reason for hiding this comment

mjkkirschner Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

mmisol Jan 26, 2021

Choose a reason for hiding this comment

mjkkirschner Jan 26, 2021

Choose a reason for hiding this comment

QilongTang Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

QilongTang Jan 26, 2021

Choose a reason for hiding this comment

mmisol Jan 26, 2021

Choose a reason for hiding this comment

QilongTang Jan 26, 2021

Choose a reason for hiding this comment

mjkkirschner commented Jan 26, 2021

QilongTang commented Jan 26, 2021 • edited Loading

mmisol commented Jan 26, 2021

mmisol left a comment

Choose a reason for hiding this comment

QilongTang commented Jan 26, 2021 •

edited

Loading

mjkkirschner Jan 26, 2021 •

edited

Loading

QilongTang Jan 26, 2021 •

edited

Loading

QilongTang commented Jan 26, 2021 •

edited

Loading