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
PEP 3121, 384 refactoring applied to elementtree module #59856
Comments
See bpo-15390 review comments :) |
Patch updated to work with current 3.4 Branch version of elementtree. |
Thanks for the patch. I'll take a look. |
I looked at the patch a bit more in depth and must admit that I'm reluctant to apply it. It's a very large patch with very little documentation about what steps are taken and why, and I just don't see the motivation. The way I see it, PEP-384 is great for compatibility of third-party extensions and embeddings of Python, but much less critical for a module that's always distributed as part of stdlib and thus is kept in exact sync with the ABI of the Python version it comes with. Correct me if I'm wrong. That said, I won't object to some refactoring if it improves the code. But when such large changes are proposed, I really prefer to see small, incremental patches that replace just a part of the code. Such patches should come with an explanation of why the change is made (i.e. which part of PEP-384 does it adhere to). |
Here is a simplified patch tackling only the PEP-3121 compliance. Eli, I think this would be good to go. |
Bless you Antoine, I've been just planning to do this myself to tackle the re-importing troubles I was having in tests the other day :-) I'll take a look at this soon, promise! |
Antoine, some questions about the patch: First, I think it omits expat_capi from the state. Is that intentional? Second, I'm not sure if this approach is fully aligned with PEP-3121. A global, shared state is still used. Instead of actually having a different module state per subinterpreter, this patch will have shared state. Another problem seems to be using PyModule_FindModule without using PyModule_AddModule first. These problems could be shared to all of Robin's original patches. Of course, there's also the possibility that I don't fully understand PEP-3121 yet :) |
What would it do in the state? There's nothing to release.
I don't understand what you are talking about. Perhaps you haven't looked |
On Wed, Aug 7, 2013 at 6:28 AM, Antoine Pitrou <report@bugs.python.org>wrote:
That's true, but I thought one of the goals of PEP-3121 is to separate
I did not look at the implementation yet. But the documentation says: """Returns the module object that was created from *def* for the current I don't see a call to PyState_AddModule. What am I missing? |
pyexpat's "capi" object is a static struct inside pyexpat.c, so that
It is called implicitly when an extension module is imported. |
Thanks Antoine. I think I understand the patch better now. Just a couple small questions and otherwise LGTM This code in the beginning in PyInit__elementtree: m = PyState_FindModule(&elementtreemodule);
if (m) {
Py_INCREF(m);
return m;
} Can you explain what use case it tries to cover? I couldn't find similar code in other modules we have that implement PEP-3121 (_csv, readline, io, etc.) This code has at least one adverse effect, for testing. The problem with re-importing _elementtree I raised in http://mail.python.org/pipermail/python-dev/2013-August/127766.html is solved by moving to PEP-3121, but this piece of code above ruins it. This is because I want to set sys.modules['pyexpat'] = None and re-import _elementtree (this is what support.import_fresh_module does). But with this code in place, if _elementtree was imported any time in the past (say, in a previous test), I'll just get the instance back without attempting to do the full module initialization.
Do you think this should be documented in the C API docs? The way they read now, it seems that calling PyState_AddModule is needed manually by extension writers. |
What I'm trying to say is that although I think I understand its effect (if the same sub-interpreter re-imports _elementtree bypassing the Python module cache, this is an additional level of caching), I'm not sure what *real* use cases it aims for. |
I don't know :-) I just re-used Robin's original patch.
Well, how to deal with module state should probably be better |
On Thu, Aug 8, 2013 at 7:00 AM, Antoine Pitrou <report@bugs.python.org>wrote:
Would you mind removing it from the patch, due to the case described above?
I'll think about it some more and will try to propose a documentation |
Yeah, I think you're right. I'll submit an updated patch or, if it's |
On Thu, Aug 8, 2013 at 7:14 AM, Antoine Pitrou <report@bugs.python.org>wrote:
Sure, I'll do that. Thanks. |
New changeset 8a060e2de608 by Eli Bendersky in branch 'default': |
Antoine, I committed your patch (with a bit of comments added), *leaving the module caching in*. This is because removing it breaks the tests, unfortunately. The _elementtree tests are so crooked that they manage to create a situation in which the module under test throws ParseError which is a different class from ET.ParseError. This is "achieved" by multiple invocations of import_fresh_module with various fresh & blocked parameters, and I still haven't fully traced all the problems yet. Since this caching only potentially harms other tests, it's ok to leave in. A longer term solution to all this will be http://mail.python.org/pipermail/python-dev/2013-August/127766.html - I want to eventually run all "monkey-patch the import environment to simulate some situation" sub-tests of ET in different subprocesses to they are kept independent. |
Thanks!
I find it useful that the test suite stresses module unloading or |
On Sat, Aug 10, 2013 at 10:38 AM, Antoine Pitrou <report@bugs.python.org>wrote:
I have no intention swiping things under the carpet. I'll get to the bottom But I still think ET tests should be logically separated into subprocesses. |
Agreed. |
Found some problems in the interaction of PEP-3121 and import_fresh_module: http://mail.python.org/pipermail/python-dev/2013-August/127862.html I'd still like to see the in-PyInit__elementtree caching deleted eventually, without harming test coverage. So this issue will remain open for now, until we decide what to do. |
BTW, Antoine, w.r.t documentation - I agree that the whole PyState_* sequence needs better documentation and examples, but in the meantime I'm attaching a simple patch for c-api/module.rst. It clarifies that PyState_AddModule doesn't really have to be called explicitly in extensions. |
Fixed by: commit a6109ef
|
Oops, I commented the wrong issue. I reopen it. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: