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

Sys.path value should be limited to the scope of the python node(CPython3 Engine) #10977

Merged
merged 4 commits into from
Aug 10, 2020

Conversation

reddyashish
Copy link
Contributor

Purpose

This PR is to address the task https://jira.autodesk.com/browse/DYN-2956.

Issue: For CPython3 Engine, when you add a custom path to the 'sys.path' variable, its value was persistent for the whole Dynamo session. This wasn't the case with IronPython2. To make this consistent, we have decided to limit the scope of this variable to individual python node.

To fix this, whenever a python node is evaluated, the 'sys.path' value is reset to contain only the default python values. For CPython engine, the first 3 paths correspond to the default python paths.

To test this, I have added a custom path "C:\Program Files\dotnet" in the Python script editor and checked that this path is not reflected in the 2nd python node. I wasn't sure if using such paths in our test could cause any issues. My thought was this dotnot folder should be present on all Windows machines with dotnet installed. Looking for suggestions.

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

@mjkkirschner @mmisol

FYIs

@DynamoDS/dynamo

@@ -188,6 +189,10 @@ static CPythonEvaluator()
{
globalScope = CreateGlobalScope();
}

// Reset the 'sys.path' value to the default python paths on node evaluation.
code = "import sys" + Environment.NewLine + "sys.path = sys.path[0:3]" + Environment.NewLine + code;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be done in its own scope. Otherwise the line numbers will be moved, which will affect traceback.

Copy link
Member

Choose a reason for hiding this comment

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

alternatively it can be done from the c# side - without injecting more code into python code string at all.

@reddyashish
Copy link
Contributor Author

reddyashish commented Aug 10, 2020

Modified the code to reset the sys.path in its own scope. Verified that the line numbers in the trace point to the correct line number.
Tried doing the same from the C# side but the scope.Get("sys.path") throws an error saying that this attribute is not present in the scope. Even the PythonEngine.PythonPath variable is also not consistent with the value present in sys.path field, so PythonEngine.PythonPath cannot be used to reset the path.

@reddyashish reddyashish added the PTAL Please Take A Look 👀 label Aug 10, 2020
@reddyashish
Copy link
Contributor Author

[Test]
public void VerifySysPathValueForCPythonEngine()
{
// open test graph
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add an assertion or another simple test to make sure the number of paths is 3 before making any changes to it? Given those paths are added by Python.NET by default, and we are assuming they are 3 on our end, this would be useful to catch changes on their end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it to the same test.

Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

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

Looks good to me

@reddyashish
Copy link
Contributor Author

Merging this.

@reddyashish reddyashish merged commit 27cb648 into DynamoDS:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTAL Please Take A Look 👀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants