Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
den-run-ai merged 22 commits intopythonnet:masterfromyagweb:add_scope
Jun 9, 2017

Conversation

yagweb
Copy link
Contributor

@yagwebyagweb commentedFeb 15, 2017
edited
Loading

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,

using (Py.GIL()){    dynamic ps = Py.CreateScope();    ps.Set("bb", 100);         //declare a variable    ps.cc = 10;                  //declare a variable dynamically    int aa = ps.Eval<int>("bb+cc+3");     //113    //add a function to the scope, and call it    //global keyword was used here to show what pyscope can do     ps.aa = 0;     ps.Exec(        "def func1():\n" +        "    global aa \n" +        "    aa = bb+cc+3 \n"                );    ps.func1(); //call the function    int aa1 = ps.Get<int>("aa");  //get variable    int aa2 = ps.aa.As<int>();     //get variable dynamically    //create a new scope and import the variables    PyScope ps2 = ps.CreateScope();    ps2.ImportModule("numpy", "np");    var aa3 = ps2.Eval<float>("np.sin(bb)");}

@@ -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 */
Copy link
Contributor

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

}

/// <summary>
/// Set the attribute of a pyobject to null.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is comment right?

@yagweb
Copy link
ContributorAuthor

@vmuriart These two comments are updated.

@den-run-ai
Copy link
Contributor

@yagweb can you update docs or readme with few examples?

@yagweb
Copy link
ContributorAuthor

yagweb commentedFeb 19, 2017
edited
Loading

@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.

@vmuriart
Copy link
Contributor

@yagweb should be clarify the intents/differences of them before adding the feature? I agree on not adding it to the documentedAPI until we hash it out though.

@den-run-ai
Copy link
Contributor

@yagweb@vmuriart I'm still struggling to understand use cases for scopes, except for storing local state.
Maybe I'm missing something? That's why I asked to add something in the docs.

@yagweb
Copy link
ContributorAuthor

yagweb commentedFeb 23, 2017
edited
Loading

@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

A scope class aims at managing the context of a interaction with Python and simplify the variable exchanging. It can act like a Python Module in .NET. and can be shared. In the Scope class, many python functions can be wrapped to operate with the local variables and global variables for easy-to-use and speeding up, such as the ImportAs method.

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
Copy link
ContributorAuthor

yagweb commentedFeb 23, 2017
edited
Loading

@denfromufa@vmuriart The most complicated thing about using pythonnet in .NET application is the GIL. This is the cause of what I said,

Currently the relationships among PythonEngine, Py, PyScope, Py.GILState, PyScope.GILState are a little messy.

Here is a relatedproposal, but I think it's impossible in some scenes.

I think there are four scenes when embedding CPython.

  1. If the application is single thread, it's no need to AcquireLock and ReleaseLock all the time, we can AcquireLock when initialize and ReleaseLock when Shutdown.

  2. In a multi-thread application, if several fast execution python functions need to called, we can treat the CPython as a shared resource, and the GIL as a lock in .NET. So a solution is wrapping each function with AcquireLock and ReleaseLock, this is what the pythonnet does right now.

  3. In a multi-thread application, if the python functions needs waiting IO and very time consuming. Then the GIL cannot be managed in .NET code. The python threading can be used and the .NET code should use asynchronous calls to interacting with Python. A similar use case is the Dispatcher of WPF.

  4. In a multi-thread application, if multi-cores are needed. Use CPython with multi-processing, or use pypy and IronPython instead.

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.

@den-run-ai
Copy link
Contributor

@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:

#330

@vmuriart
Copy link
Contributor

Even for the 1st scene thegc andFinalize inclassderived.cs might throw a wrench into things.

Looks like might need to rebase this one since#389 was merged in.

@yagweb
Copy link
ContributorAuthor

@denfromufa I see, thank you.
When users need multi-core parallel in embedded CPython, they should design their code with any means including Cython, Numba etc. It’s out of pythonnet. So the 4th scene can be deleted from this list.

@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?

@codecov
Copy link

codecovbot commentedFeb 24, 2017
edited
Loading

Codecov Report

Merging#381 intomaster willdecrease coverage by0.21%.
The diff coverage is58.79%.

Impacted file tree graph

@@            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
FlagCoverage Δ
#setup_linux75.71% <ø> (ø)⬆️
#setup_windows67.68% <58.79%> (-0.2%)⬇️
Impacted FilesCoverage Δ
src/runtime/runtime.cs88.14% <ø> (ø)⬆️
src/runtime/pythonengine.cs78.69% <100%> (+0.86%)⬆️
src/runtime/pyobject.cs37.71% <20%> (+2.35%)⬆️
src/runtime/pyscope.cs57.92% <57.92%> (ø)

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatece14424...b9dcdac. Read thecomment docs.

@yagwebyagweb mentioned this pull requestFeb 24, 2017
@den-run-ai
Copy link
Contributor

@denfromufa@vmuriart May it be better to make the PyScope class independent from the GIL solution?

I think it is safer that GIL is held inside PyScope by default. You can maybe pass non-default argument to release the GIL.

@vmuriart
Copy link
Contributor

@yagweb just finished reviewing thispr and makes sense what you're proposing. I think there's some refactoring we can do to clean up the code and there's a couple tests I want to run (like what happens if we create twoPyScope instead of aPyScope and aSubScope) (does changing a variable in the subscope that exists on the parent scope modify it).

@vmuriart
Copy link
Contributor

Looks like there is agc issue lingering somewhere.

https://travis-ci.org/pythonnet/pythonnet/jobs/204901628#L1606

@den-run-ai
Copy link
Contributor

@vmuriart how about adding this?

#400 (comment)

{
return;
}
Runtime.XDecref(globals);
Copy link
Contributor

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.

@vmuriart
Copy link
Contributor

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.

@vmuriart
Copy link
Contributor

Rebased and updated some of the internal changes

@yagweb
Copy link
ContributorAuthor

yagweb commentedApr 27, 2017
edited
Loading

@denfromufa One of@filmor's suggestions is,

The function names for accessing the variables are a bit too verbose for my taste, Get and Set would be enough.

My response is,

I agreed the names are too verbose, but these names and the Exception messages all came from IronPython. Just to be compatible.

Personally I also prefer to the shorter namesGet,Set,Remove andContains.

In addition,Executecan be renamed toExec,AddVariablescan be renamed toImportDict. Or maybe theImportDict,ImportModule, andImportScopecan all be renamed toImport.

So, compatible with IronPython or more concise, what's your opinion?

@JimSEOW
Copy link

@yagweb will the scope class address the GET, SET issues discussedhere

@yagweb
Copy link
ContributorAuthor

@JimSEOW I put my modificationshere. If your have any other questions, please talk it there.

JimSEOW reacted with thumbs up emoji

@den-run-ai
Copy link
Contributor

@yagweb regarding your previouscomment I would go for more concise,@tonyroberts@vmuriart@filmor do you agree?

@JimSEOW
Copy link

JimSEOW commentedApr 29, 2017
edited
Loading

@yagweb @denfromufa etc.
When Xamarin create .NET binding to C++ libraries, they try their best to .dotNETfy the binding using semantics that are characteristics to the .NET ecosystem.
Perhaps we can do the same here.
=> Get and Set are more the typical Java syntax for properties (not sure how easy it is to replace them with alternatives)
=> I prefer Execute to Exec
=> We are addressing users from both .NET and python ecosystems. It will be interesting to strike a balance. However, we are talking about Python 4 .NET here. What can we learn from Ironpython to suggest some naming suggestions here?

@den-run-ai
Copy link
Contributor

@filmor@vmuriart I would like to use this API during my upcoming presentation on June 8th at Microsoft. Please review and comment, if not accepting!

@vmuriart
Copy link
Contributor

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.

@@ -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>();
Copy link
Member

Choose a reason for hiding this comment

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

Remove the cast.

var result = scope.GetVariable<int>("aa");
Assert.AreEqual(113, result);

scope.Dispose();
Copy link
Member

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?


private bool isDisposed;
internal readonly IntPtr obj;
Copy link
Member

Choose a reason for hiding this comment

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

Document the internal fields.


internal PyScope(string name)
private bool isDisposed;
Copy link
Member

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);
Copy link
Member

Choose a reason for hiding this comment

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

Very nice :)

Copy link
Contributor

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? 🤔

{
value = default(T);
return true;
}
Copy link
Member

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.

}
if (name != null && NamedScopes.ContainsKey(name))
{
throw new PyScopeException($"PyScope '{name}' has existed");
Copy link
Member

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"

{
throw new ArgumentNullException(nameof(name));
}
if (name != null && NamedScopes.ContainsKey(name))
Copy link
Member

Choose a reason for hiding this comment

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

Contains(name)

Copy link
ContributorAuthor

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?

Copy link
Member

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.

{
return NamedScopes[name];
}
throw new PyScopeException($"PyScope '{name}' not exist");
Copy link
Member

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"?

{
PyScope value;
NamedScopes.TryGetValue(name, out value);
return value;
Copy link
Member

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.

@filmor
Copy link
Member

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
Copy link
ContributorAuthor

@filmor Very thank you for your detailed and valuable comments.

@denfromufa@filmor@vmuriart The last question is, should the methods 'GetVariable', 'SetVariable' etc. be renamed to shorter ones as 'Get', 'Set' etc.?

@den-run-ai
Copy link
Contributor

@yagweb i like the shorter names Get & Set.

@yagweb
Copy link
ContributorAuthor

The names 'Get & Set' etc. make sense, because variables are the only content a scope managed.
An example of this design is the 'Add' method of the List.

Methods are renamed and documents are added.

@den-run-ai
Copy link
Contributor

@vmuriart i'm planning to merge this on Wednesday, let us know if you have any feedback before that.@yagweb great work, thanks for enduring so many iterations of reviews.

@den-run-aiden-run-ai merged commit550a027 intopythonnet:masterJun 9, 2017
@yagwebyagweb deleted the add_scope branchJune 9, 2017 23:33
testrunner123 pushed a commit to testrunner123/pythonnet that referenced this pull requestSep 22, 2017
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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@den-run-aiden-run-aiden-run-ai approved these changes

@vmuriartvmuriartvmuriart left review comments

@filmorfilmorfilmor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@yagweb@den-run-ai@vmuriart@filmor@JimSEOW

[8]ページ先頭

©2009-2025 Movatter.jp