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

Remove unneeded Py_mod_create #1923

Closed
wants to merge 1 commit into from
Closed

Remove unneeded Py_mod_create #1923

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 10, 2017

While thinking about how to implement PEP 547 (direct execution using -m switch), I have taken a look into cython and it seems that it doesn't need Py_mod_create at all.
The only thing the create slot was used for was setting of module attributes, which are accessible from the module's __spec__.
These attributes can be easily set from the exec slot, hence, the 'Py_mod_create' slot can excluded.

In CPython issue 30403 you mention that a custom module type might also be needed for C level module globals. These can, however, be kept in the module state.

Mod attributes are now set in exec slot.
@@ -2299,6 +2299,13 @@ def generate_module_init_func(self, imported_modules, env, code):

code.put_declare_refcount_context()
code.putln("#if CYTHON_PEP489_MULTI_PHASE_INIT")
code.putln('PyObject *moddict, *spec;')
code.putln('moddict = PyModule_GetDict(__pyx_pyinit_module);')
code.putln('spec = PyDict_GetItemString(moddict, "__spec__");')
Copy link
Contributor

Choose a reason for hiding this comment

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

These calls lack error handling. And I'd also prefer having this setup in an external C helper function (as before), rather than generating it line by line. It's just easier to maintain that way.

@scoder
Copy link
Contributor

scoder commented Oct 10, 2017

Thanks, I agree that it's reasonable to do it this way.

@scoder
Copy link
Contributor

scoder commented Oct 10, 2017

However, the problematic bit is that Cython modules do not currently have their module state in non-global storage, and some modules will never support that, e.g. if they depend on external libraries that have global state. That makes it an opt-in feature, not something we can enable by default.

So, all we can really do currently is to reuse the module object and prevent instantiating additional ones, i.e. prevent the library from being reinitialised. And that can be done, although in a highly hackish way, by implementing create and returning the original module instance.

I've already started looking into removing the global state, but that's not easy and will probably slow some things down. For the time being, we only have two choices: not support PEP-489 at all, or implementing Py_mod_create and preventing re-initialisation in it.

@scoder
Copy link
Contributor

scoder commented Oct 10, 2017

I'll therefore close this PR as "you're right, but it's not actually helpful".

@scoder scoder closed this Oct 10, 2017
scoder added a commit to scoder/cython that referenced this pull request Oct 10, 2017
…EP-489 module create function, which will eventually be removed.

See cython#1923.
@encukou
Copy link
Contributor

encukou commented Oct 11, 2017

I would choose not supporting PEP-489 at all until global state can be removed. The PEP itself has language to support this.

As for external libraries with global state, there will need to be a Python-wide discussion – e.g. if readline supports only one global callback, how should the Python wrapper work if it's loaded more than once?
Even on that front, the "removing global state" effort is not easy and might slow some things down. But I'd like to align that work with Cython as much as possible.

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.

2 participants