- Notifications
You must be signed in to change notification settings - Fork752
add a scope class to manage the context of interaction with Python an…#381
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/runtime/pythonengine.cs Outdated
@@ -439,7 +454,7 @@ public static PyObject ModuleFromString(string name, string code) | |||
borrowedLocals = false; | |||
} | |||
var flag = (IntPtr)257; /* Py_file_input */ | |||
var flag = (IntPtr)_flag; /* Py_file_input */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The comment here should be updated
src/embed_tests/pyscope.cs Outdated
} | ||
/// <summary> | ||
/// Set the attribute of a pyobject to null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is comment right?
@vmuriart These two comments are updated. |
@yagweb can you update docs or readme with few examples? |
yagweb commentedFeb 19, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@denfromufa The unit tests in "src/embed_tests/pyscope.cs" are commented and can be used as examples. Currently the relationships among PythonEngine, Py, PyScope, Py.GILState, PyScope.GILState are a little messy. The APIs may changes at any time, so I may not add these examples into docs or readme until the APIs become stable. We can use the unit tests instead. |
@yagweb should be clarify the intents/differences of them before adding the feature? I agree on not adding it to the documented |
yagweb commentedFeb 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@vmuriart I am sorry for that, the modifications are too many last time. Last time, the PR is cleaned up by separating a new PR ‘add eval and exec’ out from it, merging two classes, renaming methods, etc. I updated all my comments and forgot to inform you. In the future, I will provide detailed comments for each commit. @denfromufa@vmuriart Storing local state is the start point. I wanted to embedding python into a C# application using pythonnet, and found the APIs for this scene are hard to use. In my another commenthere, I concluded that
I just make a new commit, now this PR implements almost all the functions of IronPython about accessing Python code from .NET code. I still don't know whether it's enough or not until it's actually used. |
yagweb commentedFeb 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@denfromufa@vmuriart The most complicated thing about using pythonnet in .NET application is the GIL. This is the cause of what I said,
Here is a relatedproposal, but I think it's impossible in some scenes. I think there are four scenes when embedding CPython.
In the multi-thread scenes, it's better to use IronPython if possible. In the 3rd scene, I think acquire GIL by default in .NET is impossible. |
@yagweb in my view the multi-threading and asynchronous execution can be outside of this PR. GIL issues are well-known in Python community. Besides multi-processing in CPython you can look at Cython with nogil contexts, and Numba with nopython mode for numerical code. PyPy-STM is not ready for production use and only available on Linux. PyPy with cpyext is currently crashing when importing pythonnet: |
Even for the 1st scene the Looks like might need to rebase this one since#389 was merged in. |
@denfromufa I see, thank you. @vmuriart Thank you. it seems like there no "real" single-threaded application since the GC working behind and needing to call CPython. So the 1st scene can also be deleted from this list. @denfromufa@vmuriart May it be better to make the PyScope class independent from the GIL solution? |
codecovbot commentedFeb 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #381 +/- ##==========================================- Coverage 68.56% 68.34% -0.22%========================================== Files 63 64 +1 Lines 5344 5560 +216 Branches 855 894 +39 ==========================================+ Hits 3664 3800 +136- Misses 1444 1496 +52- Partials 236 264 +28
Continue to review full report at Codecov.
|
I think it is safer that GIL is held inside PyScope by default. You can maybe pass non-default argument to release the GIL. |
@yagweb just finished reviewing this |
Looks like there is a https://travis-ci.org/pythonnet/pythonnet/jobs/204901628#L1606 |
@vmuriart how about adding this? |
src/runtime/pythonengine.cs Outdated
{ | ||
return; | ||
} | ||
Runtime.XDecref(globals); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
gc
issue is most likely here. You'll need to check that the engine is initialize and that it isn't finalizing before running these.
That wouldn't fix it. The issue is that the engine get shutdown earlier at "some point" and looks like currently there is nothing checking the status of the engine before trying to run more commands on it. |
Rebased and updated some of the internal changes |
yagweb commentedApr 27, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@denfromufa One of@filmor's suggestions is,
My response is,
Personally I also prefer to the shorter names In addition, So, compatible with IronPython or more concise, what's your opinion? |
JimSEOW commentedApr 27, 2017
@yagweb regarding your previouscomment I would go for more concise,@tonyroberts@vmuriart@filmor do you agree? |
JimSEOW commentedApr 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@yagweb @denfromufa etc. |
I'll review over the weekend. I don't think this one introduced any GC bugs but im seeing a bunch of warnings showing up on the Python Tests now (though I suspect its unrelated to this but want to check). I'll review@filmor comments to make sure requests were resolved. |
src/embed_tests/TestPyScope.cs Outdated
@@ -170,7 +170,7 @@ public void TestImportModule() | |||
ps.Exec("sys.attr1 = 2"); | |||
var value1 = ps.Eval<int>("sys.attr1"); | |||
var value2 = (int)sys.attr1.AsManagedObject(typeof(int)); | |||
var value2 = (int)sys.attr1.As<int>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove the cast.
src/embed_tests/TestPyScope.cs Outdated
var result = scope.GetVariable<int>("aa"); | ||
Assert.AreEqual(113, result); | ||
scope.Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Useusing
to give a good example?
src/runtime/pyscope.cs Outdated
private bool isDisposed; | ||
internal readonly IntPtr obj; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Document the internal fields.
src/runtime/pyscope.cs Outdated
internal PyScope(string name) | ||
private bool isDisposed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Private should be_isDisposed
.
{ | ||
name = ""; | ||
} | ||
var module = Runtime.PyModule_New(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Very nice :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i'm still pondering what did you notice here so special@filmor? 🤔
src/runtime/pyscope.cs Outdated
{ | ||
value = default(T); | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This means that if someone doesTryGetVariable<int>("something", out val)
wheresomething = None
, they will get as a resultval = 0
without any further indication. Seems like a very bad idea for value types.
src/runtime/pyscope.cs Outdated
} | ||
if (name != null && NamedScopes.ContainsKey(name)) | ||
{ | ||
throw new PyScopeException($"PyScope '{name}' has existed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"A scope of name '{name}' does already exist"
src/runtime/pyscope.cs Outdated
{ | ||
throw new ArgumentNullException(nameof(name)); | ||
} | ||
if (name != null && NamedScopes.ContainsKey(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Contains(name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I'm not quite understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You already defined a member functionContains
, use that one.
src/runtime/pyscope.cs Outdated
{ | ||
return NamedScopes[name]; | ||
} | ||
throw new PyScopeException($"PyScope '{name}' not exist"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe"There is no scope named '{name}' registered in this manager"
?
src/runtime/pyscope.cs Outdated
{ | ||
PyScope value; | ||
NamedScopes.TryGetValue(name, out value); | ||
return value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As said above, just forward the semantics ofTryGetValue
directly.
Regarding doing things like IronPython: I doubt that we actually want API parity on the .NET side, for that we are too far away. I also don't see much of a benefit, pythonnet is not built on top of the scripting architecture used by IronPython but directly on Python's API, so they can not be replacements for each other anyhow. This is completely disconnected from the discussion how thePython side should look like. |
@yagweb i like the shorter names Get & Set. |
The names 'Get & Set' etc. make sense, because variables are the only content a scope managed. Methods are renamed and documents are added. |
pythonnet#381)* add a scope class to manage the context of interaction with Python and simplify the variable exchanging* Deprecate public RunStringHad to remove defaults to disambiguate call on `internal RunString`.Can re-add after removing `public RunString`Closespythonnet#401* Rename several methods and add three methodsReferring to IronPython, change the name of the methods Get, Exists, SetLocal, DelLocal to GetVariable, ContainsVariable, SetVariable, RemoveVariable.Hidden the methods SetGlobalVariable, RemoveGlobalVariable.Add a new method 'Compile' to compile string into ast, the ast can be seen as the ScriptSource of IronPython.Add two new methods 'Execute' and 'Execute<T>' to execute an ast andobtain the result, corresponding to the 'Execute' method of IronPython.* rebased* Rebased update* format cleanup* create unnamed pyscope, make PyScope.GILState saveremove method GetInstHandleadd function to create unnamed pyscopeadd a field isDisposed for PyScope.GILState to make it more save* fixup! create unnamed pyscope, make PyScope.GILState save* remove GIL and rebased* Add several methodsadd ImportScope: a scope can import variable from any scope, equivalent topython 'import * from mod'add dynamic member supportadd an OnDispose eventremove the field ‘globals’ referring to python moduleput the scope class in a new fileUnit test:TestThread uses scope function replacing Exec/Eval to speed up the execution.* add a Variables method* fixup! add a Variables method* remove private method _GetVariable* add unit test for Variables() method* add several methods and rebasedAdd an optional dict parameter for Eval/Exec/Execute methodsAdd a new field obj which point to a Python Module (same as pyobject)Add a static method NewRename the old ImportScope method to ImportAllFromScopeAdd a new ImportScope method and add a unit test for itRename the CreateScope method to NewScope* add a new class PyScopeManager* cleaned up the Import methods* updated according to filmor's comments* fixup! updated according to filmor's comments* Get/Set Methods renamed
Uh oh!
There was an error while loading.Please reload this page.
It‘s a prototype of this proposalhere to help its discussion.
Modifications are located at file pythonengine.cs, A class named PyScope was added for the variable management and exchanging between .NET and Python.
Several unit tests were given in a new file "src/embed_tests/pyscope.cs" which can be used as examples.
One of the example,