Skip to content

added missing PyThreadState_Swap to Runtime and its Delegates #1673

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

Closed
wants to merge 1 commit into from

Conversation

eirannejad
Copy link
Contributor

What does this implement/fix? Explain your changes.

PyThreadState_Swap was missing in Runtime and Runtime.Delegates code

Does this close any currently open issues?

No

Any other comments?

N/A

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
    ⚠️ Skipping tests since there are not any tests for specific runtime p/invokes. This method is not used anywhere in original pythonnet code base as of creating this PR
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@lostmsu
Copy link
Member

lostmsu commented Jan 15, 2022

This looks good but without usages there's not much point merging it. We only have P/Invoke signatures for functions that are used by Python.Runtime.

@eirannejad
Copy link
Contributor Author

@lostmsu I'm using this in a fork that I have working with sub-interpreters. Whichever you decide. I thought it's better to PR to master

@lostmsu
Copy link
Member

lostmsu commented Jan 15, 2022

Are you planning to upstream subinterpreters? Or is it not yet ready?

@lostmsu
Copy link
Member

lostmsu commented Jan 15, 2022

We are about to release 3.0 with breaking changes. If subinterpreters require more breaking changes, it might make sense to try getting them in, otherwise you might need to wait until 4.0

@eirannejad
Copy link
Contributor Author

eirannejad commented Jan 15, 2022

@lostmsu I definitely am. To be honest I'm a little in the dark as to what is the overall scope and design goal for pythonnet. I'd appreciate your help and info on this. It'd help me push more generic code upstream.

Q: Is PythonEngine intended to be a facade over the runtime and wrap more complex functionality, simplifying runtime usage? or is it intended to be as close as possible to python runtime itself? Runtime.cs currently wraps Py_NewInterpreter and Py_EndInterpreter without them being used in pythonnet code AFAIK. I'm not sure if I should just wrap these in PythonEngine or provide a context manager like this to simplify usage?

  • The linked code is custom to my specific use case but I'd love to make it more generic and push upstream
  • It's more of an overall design api goal question really.

@lostmsu
Copy link
Member

lostmsu commented Jan 15, 2022

Actually, this passage does not look promising:

Furthermore, extensions (such as ctypes) using these APIs to allow calling of Python code from non-Python created threads will probably be broken when using sub-interpreters.

I wonder what is your use case? Is PyScope not sufficient?

@eirannejad
Copy link
Contributor Author

eirannejad commented Jan 15, 2022

@lostmsu I'm embedding pythonnet in a dotnet application and creating a cpython IDE over it. I need to run a mini language server on the python engine as well that doesn't have anything to do with the actual user scripts being executed by the engine. Effectively this creates two completely independent usages for pythonnet under the same process. I need to create a sub-interpreter for these reasons:

  • The language server imports lots of support libraries for linting and autocompletion. If this is just another PyScope in the main interpreter it would sprinkle sys.modules with those imported modules
  • I need to assign independent stdout and stderr streams to this language server and therefore need a separate interpreter

Please let me know if there is a better way to do this

P.S. I don't see PyScope in pythonnet3 anymore. Seems like it is PyModule now right?

@filmor
Copy link
Member

filmor commented Jan 18, 2022

Regarding PythonEngine:

We will have to break this one eventually anyhow if we ever want subinterpreters, so if you want to make a proposal, go ahead. From my perspective, it only exists for compatibility reasons, all newer functionalities were added to the relatively ergonomic Py class. I'd suggest leaving PythonEngine as is (marking it as obsolete) and creating a new class PyInterpreter or PySubInterpreter to carry the Eval and Import etc. functions on an instance instead of static and just pointing Py.Eval etc. to a Py.MainInterpreter.

Regarding PyScope:

This is indeed PyModule now, there was a lot of overlap: #1569

@lostmsu
Copy link
Member

lostmsu commented Jan 18, 2022

My understanding is that the issue requires more involved changes.

For modules using single-phase initialization, e.g. PyModule_Create() (this is what Python.NET does AFAIR), the first time a particular extension is imported, it is initialized normally, and a (shallow) copy of its module’s dictionary is squirreled away. When the same extension is imported by another (sub-)interpreter, a new module is initialized and filled with the contents of this copy; the extension’s init function is not called. Objects in the module’s dictionary thus end up shared across (sub-)interpreters, which might cause unwanted behavior (see Bugs and caveats below).

Now if you think about it, the modules that Python.NET creates to reflect .NET namespaces if simply copied to the new subinterpreter won't get notified of newly loaded assemblies.

The following passage is also unclear to the exact effect:

Also note that combining this functionality with PyGILState_* APIs is delicate, because these APIs assume a bijection between Python thread states and OS-level threads, an assumption broken by the presence of sub-interpreters. It is highly recommended that you don’t switch sub-interpreters between a pair of matching PyGILState_Ensure() and PyGILState_Release() calls. Furthermore, extensions (such as ctypes) using these APIs to allow calling of Python code from non-Python created threads will probably be broken when using sub-interpreters.

We can probably defer that issue to users, but the problem is that because most usages of Python.NET are higher-level, we will get a lot of people asking why it does not work like they expect it should.

BTW, @eirannejad, according to the first quote above in the example you mentioned, Python.NET will only be initialized once.

As for running a language server, why not use Language Server Protocol used in VS Code for all languages. Then you should be able to just reuse existing Python language server that Microsoft developed for VS Code. This does not use subinterpreters, and instead just runs language server library in a separate process.

AFAIK, Python language server in VS Code does not autocomplete .NET types, and this is something we might want to address instead.

@lostmsu
Copy link
Member

lostmsu commented May 7, 2022

As discussed, we are not taking it in the current form. I hope the LSP is good alternative for you @eirannejad

@lostmsu lostmsu closed this May 7, 2022
@eirannejad eirannejad deleted the dev/fixmissingswap branch July 2, 2022 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants