add mechanism to reload Cpython modules and mark graph dirty #11714

mjkkirschner · 2021-05-27T01:54:53Z

Purpose

https://jira.autodesk.com/browse/DYN-3514
https://jira.autodesk.com/browse/DYN-2941

Update:

To help dynamo users who write their own python modules in cpython deal with the difference in module import/reload between cpython and ironPython- this PR adds a button to Reset CPython. What it does is to reload python modules that are backed by a .py file and are not named __main__ and a few other checks inspired by autoreload
https://ipython.org/ipython-doc/3/config/extensions/autoreload.html

The implementation is a subset of what autoreload does:

modules that have .py files are reloaded - modules that are reloaded, are logged to the console:
ie:

2021-05-28 22:18:42Z : Python Script: considering reload_test3
2021-05-28 22:18:42Z : Python Script: reloading reload_test3
2021-05-28 22:18:42Z : Python Script: failed to reload reload_test3 spec not found for the module 'reload_test3'
2021-05-28 22:18:42Z : Python Script: considering importlib._bootstrap
2021-05-28 22:18:42Z : Python Script: considering importlib._bootstrap_external
2021-05-28 22:18:42Z : Python Script: considering importlib
2021-05-28 22:18:42Z : Python Script: considering importlib.machinery
2021-05-28 22:18:42Z : Python Script: considering importlib.abc
2021-05-28 22:18:42Z : Python Script: considering contextlib
2021-05-28 22:18:42Z : Python Script: considering importlib.util
2021-05-28 22:18:42Z : Python Script: considering pprint

Currently, after the modules are reloaded, the Dynamo engine is reset and all nodes are marked dirty - this is very likely overkill. I think we would be safe simply marking the nodes dirty in topological order or disabling execution, marking all nodes dirty, and then re-enabling execution.
The goal is to have the DS GC collect the old objects created with out dated modules, the easiest way to do this is to rerun the graph.

also:

I had to change the use of python thread macros and lock code we started using in this PR: Tests for CPython3 Engine as well as for IronPython. #10631 - I think it was a bit confused (passing a pointer from the GIL mutex to a macro that expected a pointer to a different mutex).
- I more closely followed the examples in this doc:
  https://github.com/pythonnet/pythonnet/wiki/Threading
- I got rid of the acquire and release lock calls as I believe using the GIL() call is analogous to that.
I ended up using static internal events to avoid having any references on DSCPython or PythonNet in DynamoCore/DynamoCoreWPF - I guess I could also use interfaces and a singleton or something. Feedback welcome there.
Modified the style for flatButton so that its center aligned @Jingyi-Wen fyi. (that affects the group add stye button)

TODO:

address todos
more tests
tooltip/docs for when to use this.
outside scope of this PR - but will create docs for addressing this problem in various ways (import.reload, reloadr, autoreload, this button) - stubbed out here: https://github.com/DynamoDS/Dynamo/wiki/Work-in-progress-to-improve-Python-3-support#import-module-works-differently-in-ironpython-and-cpython-wip
update license and rtf files to note reference of autoreloader from Ipython project.

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

raise event from button handle event in cpython engine fix python thread macros - no need for double lock. mixing thread state and gil state. shutdown when requested fix tests.

Jingyi-Wen · 2021-05-27T15:31:11Z

@mjkkirschner The location of the restart button makes sense! For the button style, could we not have the outline?
In HIG, the default state of the flat button is without outline (so should look the same as the "Add style" button).

mjkkirschner · 2021-05-27T15:48:13Z

@mjkkirschner The location of the restart button makes sense! For the button style, could we not have the outline?
In HIG, the default state of the flat button is without outline (so should look the same as the "Add style" button).

Hey @Jingyi-Wen yep it looks just like the Add Style button - I used the same style, I was just hovering over it when I took the screenshot.

Jingyi-Wen · 2021-05-27T16:00:45Z

@mjkkirschner The location of the restart button makes sense! For the button style, could we not have the outline?
In HIG, the default state of the flat button is without outline (so should look the same as the "Add style" button).

Hey @Jingyi-Wen yep it looks just like the Add Style button - I used the same style, I was just hovering over it when I took the screenshot.

Got it. Looks good then!

resx for add style resx for restart resx for tooltip on restart button

logging re enabled

add failing test modify tests to use different module names to avoid moving on disk error - missing mod spec

remove extra python

change tooltips and button text update tests fix bug with engine name

mjkkirschner · 2021-06-01T20:32:22Z

src/DynamoCore/Models/DynamoModel.cs

@@ -119,125 +119,6 @@ public partial class DynamoModel : IDynamoModel, IDisposable, IEngineControllerM

        #endregion

-        #region events


moved all DynamoModel events to DynamoModelEvents.cs file.

mjkkirschner · 2021-06-01T20:33:41Z

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

@@ -2390,4 +2390,13 @@ Uninstall the following packages: {0}?</value>
  <data name="PreferencesViewEnableNodeAutoCompleteTooltipText" xml:space="preserve">
    <value>Learn more about Node Autocomplete feature.</value>
  </data>
+  <data name="AddStyleButton" xml:space="preserve">
+    <value>Add Style</value>


this was missing a resx property.

src/Libraries/DSCPython/CPythonEvaluator.cs

mjkkirschner · 2021-06-01T20:38:43Z

src/DynamoCore/Models/DynamoModelEvents.cs

+            if (CurrentWorkspace is HomeWorkspaceModel hmwsm)
+            {
+                RequestPythonReset?.Invoke(pythonEngine);
+                ResetEngine(true);


I think this is probably overkill, but this function is only triggered manually for now.

by manually you mean via new button in preferences ?
When/if we move to something more reactive/automated....we will probably want to reset only the nodes that are affected by the .py reloads right ?

Yes, that would be the ideal - BUT identifying those nodes without creating a circular reference is kind of a pain - we can use reflection or string matching if necessary.

so by re executing all the nodes we are also taking care (refreshing) of potentially dangling objects that might still reference the old python files right ?

src/Libraries/DSCPython/CPythonEvaluator.cs

pinzart90 · 2021-06-02T19:40:44Z

src/DynamoCoreWpf/Properties/Resources.resx

+    <value>Reset CPython</value>
+  </data>
+  <data name="ResetCPythonButtonToolTip" xml:space="preserve">
+    <value>Resets CPython environment by reloading modules.</value>


does this need to be more descriptive ?
all modules ? or some ? do we need to be specific I wonder ...

thanks @pinzart90 - I was thinking to leave it a bit intentionally vague at this point so we can adjust it easily, but we could specify modules that have file attributes?

not sure....maybe vague here and more specific in some dynamoDS wiki ?

I've added https://github.com/DynamoDS/Dynamo/wiki/Work-in-progress-to-improve-Python-3-support#import-module-works-differently-in-ironpython-and-cpython-wip for docs, we can expand on specifics there and easily update it.

pinzart90 · 2021-06-02T19:44:36Z

src/Libraries/DSCPython/CPythonEvaluator.cs

-
-            IntPtr gs = PythonEngine.AcquireLock();
-            try
-            {


this lock stuff is not needed anymore ?

nope, I believe this is the same as using Py.GIL() - see the description and threading wiki that says as much.

fix issue with static logger reference

pinzart90 · 2021-06-02T19:51:25Z

src/Libraries/DSCPython/CPythonEvaluator.cs

+import importlib.util
+import os
+def getInfoFile(module):
+    if not hasattr(module, '__file__') or module.__file__ is None:


maybe add comments to all these filters/conditions so that we know what the intention was (in case something goes wrong)

good idea, will add some comments

where did you find documentation for these module attributes ?

Maybe we should add a comment (like in the importlib.reload documentation)
Reloading sys, __main__, builtins and other key modules is not recommended
https://docs.python.org/3.4/library/importlib.html#importlib.reload

will do - these attributes (and this function) were modified from the autoreload.py script which is why I added attribution to this library in the license and about boxes.

pinzart90 · 2021-06-02T20:01:05Z

src/Libraries/DSCPython/CPythonEvaluator.cs

+    return py_filename
+
+
+for modname,mod in sys.modules.copy().items():


any idea why we need to make the copy() call ?

yes because iterating over sys.modules and reloading modules directly modifies the keys in this dictionary during iteration.

Making a copy avoids modifying the dictionary we're iterating over.

update comments in reload snippet

mjkkirschner · 2021-06-03T16:17:10Z

@pinzart90 PTAL - I've added comments, please check if they are adequate. When this is merged, I will update the wiki with specifics and note that the change has been merged.

mjkkirschner · 2021-06-03T20:28:53Z

@pinzart90 this passed on the parallel job here:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/2562/
and serial one here:
https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/797/

though there were some flaky test failures we've seen before that occurred on the previous parallel run. I'm going to merge this, but if we see them get more frequent we can revert.
FYI @QilongTang @jasonstratton

add new restart cpython button

e36c3c0

raise event from button handle event in cpython engine fix python thread macros - no need for double lock. mixing thread state and gil state. shutdown when requested fix tests.

mjkkirschner added the WIP label May 27, 2021

mjkkirschner added 2 commits May 27, 2021 10:46

fix test

0df76a4

fix tst paths

39cabdd

mjkkirschner added 8 commits May 27, 2021 17:30

move events to model events .cs - just some cleanup

51eb564

resx for add style resx for restart resx for tooltip on restart button

add analytics

f242ab1

logging re enabled

add module reload on button press

702e5d0

add failing test modify tests to use different module names to avoid moving on disk error - missing mod spec

missing friend assembly

e8b71f7

add new test

809a9db

remove extra python

add test in auto run mode

7d77f6d

change names to reset

c03b8c0

change tooltips and button text update tests fix bug with engine name

typo

6960bfb

mjkkirschner changed the title ~~WIP add a restart cpython engine button to python prefs~~ add a reset cpython button to python prefs Jun 1, 2021

mjkkirschner changed the title ~~add a reset cpython button to python prefs~~ add mechanism to reload Cpython modules and mark graph dirty Jun 1, 2021

mjkkirschner commented Jun 1, 2021

View reviewed changes

src/Libraries/DSCPython/CPythonEvaluator.cs Outdated Show resolved Hide resolved

mjkkirschner requested review from pinzart90 and sm6srw June 1, 2021 20:37

mjkkirschner added PTAL Please Take A Look 👀 and removed WIP labels Jun 1, 2021

mjkkirschner commented Jun 1, 2021

View reviewed changes

mjkkirschner commented Jun 2, 2021

View reviewed changes

src/Libraries/DSCPython/CPythonEvaluator.cs Outdated Show resolved Hide resolved

mjkkirschner added 2 commits June 2, 2021 12:56

cleanup

b249b8e

update licenses

ebed648

pinzart90 reviewed Jun 2, 2021

View reviewed changes

mjkkirschner added 2 commits June 2, 2021 15:45

deal with some old todos

0b0c71a

fix issue with static logger reference

revert extra files

9c88926

pinzart90 reviewed Jun 2, 2021

View reviewed changes

rename handler

860bbfb

update comments in reload snippet

Merge remote-tracking branch 'upstream' into pythonResetManual

2cd3701

pinzart90 approved these changes Jun 3, 2021

View reviewed changes

mjkkirschner merged commit 38b57f9 into DynamoDS:master Jun 3, 2021

mjkkirschner deleted the pythonResetManual branch June 3, 2021 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add mechanism to reload Cpython modules and mark graph dirty #11714

add mechanism to reload Cpython modules and mark graph dirty #11714

mjkkirschner commented May 27, 2021 •

edited

Loading

Jingyi-Wen commented May 27, 2021

mjkkirschner commented May 27, 2021

Jingyi-Wen commented May 27, 2021

mjkkirschner Jun 1, 2021 •

edited

Loading

mjkkirschner Jun 1, 2021

mjkkirschner Jun 1, 2021

pinzart90 Jun 2, 2021

mjkkirschner Jun 2, 2021

pinzart90 Jun 2, 2021

mjkkirschner Jun 3, 2021

pinzart90 Jun 2, 2021

mjkkirschner Jun 2, 2021

pinzart90 Jun 2, 2021

mjkkirschner Jun 3, 2021 •

edited

Loading

pinzart90 Jun 2, 2021

mjkkirschner Jun 2, 2021

pinzart90 Jun 2, 2021

mjkkirschner Jun 2, 2021

pinzart90 Jun 2, 2021

pinzart90 Jun 2, 2021

mjkkirschner Jun 3, 2021

pinzart90 Jun 2, 2021

mjkkirschner Jun 3, 2021

mjkkirschner commented Jun 3, 2021

mjkkirschner commented Jun 3, 2021

		@@ -119,125 +119,6 @@ public partial class DynamoModel : IDynamoModel, IDisposable, IEngineControllerM

		#endregion

		#region events

		return py_filename


		for modname,mod in sys.modules.copy().items():

add mechanism to reload Cpython modules and mark graph dirty #11714

add mechanism to reload Cpython modules and mark graph dirty #11714

Conversation

mjkkirschner commented May 27, 2021 • edited Loading

Purpose

Update:

Declarations

Jingyi-Wen commented May 27, 2021

mjkkirschner commented May 27, 2021

Jingyi-Wen commented May 27, 2021

mjkkirschner Jun 1, 2021 • edited Loading

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

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 Jun 3, 2021 • edited Loading

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

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 Jun 3, 2021

mjkkirschner commented Jun 3, 2021

mjkkirschner commented May 27, 2021 •

edited

Loading

mjkkirschner Jun 1, 2021 •

edited

Loading

mjkkirschner Jun 3, 2021 •

edited

Loading