[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Irit Katriel]

Change by Irit Katriel :


--
resolution:  -> duplicate
status: open -> closed
superseder:  -> PEP 3121, 384 refactoring applied to elementtree module

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[STINNER Victor]

STINNER Victor  added the comment:

> Fixed by: (...) bpo-1635741: Convert _sre types to heap types...

Oops, I commented the wrong issue. I reopen it.

--
resolution: duplicate -> 
status: closed -> open
superseder: Py_Finalize() doesn't clear all Python objects at exit -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[STINNER Victor]

STINNER Victor  added the comment:

Fixed by:

commit a6109ef68d421712ba368ef502c4789e8de113e0
Author: Erlend Egeberg Aasland 
Date:   Fri Nov 20 13:36:23 2020 +0100

bpo-1635741: Convert _sre types to heap types and establish module state 
(PEP 384) (GH-23393)

--
nosy: +vstinner
resolution:  -> duplicate
stage: patch review -> resolved
status: open -> closed
superseder:  -> Py_Finalize() doesn't clear all Python objects at exit

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Roundup Robot]

Roundup Robot added the comment:

New changeset 8a060e2de608 by Eli Bendersky in branch 'default':
Issue #15651: PEP 3121 refactoring for _elementtree
http://hg.python.org/cpython/rev/8a060e2de608

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 Antoine, I committed your patch (with a bit of comments added),
 *leaving the module caching in*.

Thanks!

 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.

I find it useful that the test suite stresses module unloading or
reloading. There's probably a bug either in ET or in the ET tests. I'm
not saying it's a very important issue of course, but IMHO it would be
better if we don't try to swipe it under the carpet :-)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

On Sat, Aug 10, 2013 at 10:38 AM, Antoine Pitrou rep...@bugs.python.orgwrote:


 Antoine Pitrou added the comment:

  Antoine, I committed your patch (with a bit of comments added),
  *leaving the module caching in*.

 Thanks!

  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.

 I find it useful that the test suite stresses module unloading or
 reloading. There's probably a bug either in ET or in the ET tests. I'm
 not saying it's a very important issue of course, but IMHO it would be
 better if we don't try to swipe it under the carpet :-)


I have no intention swiping things under the carpet. I'll get to the bottom
of this to understand the exact flow that causes this to happen.

But I still think ET tests should be logically separated into subprocesses.
If we want to stress test module unloading and reloading, let's have
specific, targeted tests for that.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 If we want to stress test module unloading and reloading, let's have
 specific, targeted tests for that.

Agreed.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Changes by Eli Bendersky eli...@gmail.com:


Added file: http://bugs.python.org/file31226/pystate-findmodule-clarify.1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

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.


 I don't see a call to PyState_AddModule. What am I missing?
It is called implicitly when an extension module is imported.

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

 Can you explain what use case it tries to cover?

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

I don't know :-) I just re-used Robin's original patch.

  I don't see a call to PyState_AddModule. What am I missing?
 It is called implicitly when an extension module is imported.
 
 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.

Well, how to deal with module state should probably be better
documented. Not sure how, though.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

On Thu, Aug 8, 2013 at 7:00 AM, Antoine Pitrou rep...@bugs.python.orgwrote:


 Antoine Pitrou added the comment:

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

 I don't know :-) I just re-used Robin's original patch.


Would you mind removing it from the patch, due to the case described above?
ISTM that in real scenarios the sys.modules cache kicks in anyway. It
should not be really bypassed for any given sub-interpreter in sane code.


   I don't see a call to PyState_AddModule. What am I missing?
  It is called implicitly when an extension module is imported.
 
  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.

 Well, how to deal with module state should probably be better
 documented. Not sure how, though.


I'll think about it some more and will try to propose a documentation
patch. This can be done incrementally; we don't have to go to perfect docs
on the first try ;-)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 Would you mind removing it from the patch, due to the case described
 above?
 ISTM that in real scenarios the sys.modules cache kicks in anyway. It
 should not be really bypassed for any given sub-interpreter in sane
 code.

Yeah, I think you're right. I'll submit an updated patch or, if it's
the only issue with it, perhaps you can simply remove the 3 offending
lines?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

On Thu, Aug 8, 2013 at 7:14 AM, Antoine Pitrou rep...@bugs.python.orgwrote:


 Antoine Pitrou added the comment:

  Would you mind removing it from the patch, due to the case described
  above?
  ISTM that in real scenarios the sys.modules cache kicks in anyway. It
  should not be really bypassed for any given sub-interpreter in sane
  code.

 Yeah, I think you're right. I'll submit an updated patch or, if it's
 the only issue with it, perhaps you can simply remove the 3 offending
 lines?


Sure, I'll do that. Thanks.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 First, I think it omits expat_capi from the state. Is that
 intentional?

What would it do in the state? There's nothing to release.

 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.

I don't understand what you are talking about. Perhaps you haven't looked
what PyState_FindModule() does?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

On Wed, Aug 7, 2013 at 6:28 AM, Antoine Pitrou rep...@bugs.python.orgwrote:


 Antoine Pitrou added the comment:

  First, I think it omits expat_capi from the state. Is that
  intentional?

 What would it do in the state? There's nothing to release.


That's true, but I thought one of the goals of PEP 3121 is to separate
states between sub-interpreters. So that one can't corrupt another. I'm not
sure how much it matters in practice in this case of the pyexpat capsule;
need to look into it more.

  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.

 I don't understand what you are talking about. Perhaps you haven't looked
 what PyState_FindModule() does?


I did not look at the implementation yet. But the documentation says:

Returns the module object that was created from *def* for the current
interpreter. This method requires that the module object has been attached
to the interpreter state with
PyState_AddModule()http://docs.python.org/dev/c-api/module.html?highlight=pymoduledef_base#PyState_AddModulebeforehand.
In case the corresponding module object is not found or has not
been attached to the interpreter state yet, it returns NULL.

I don't see a call to PyState_AddModule. What am I missing?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 That's true, but I thought one of the goals of PEP 3121 is to
 separate
 states between sub-interpreters. So that one can't corrupt another.
 I'm not
 sure how much it matters in practice in this case of the pyexpat
 capsule;
 need to look into it more.

pyexpat's capi object is a static struct inside pyexpat.c, so that
wouldn't change anything.
Separating states between sub-interpreters only matters when said state
is mutable, which it isn't here.

 I don't see a call to PyState_AddModule. What am I missing?

It is called implicitly when an extension module is imported.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

Here is a simplified patch tackling only the PEP 3121 compliance. Eli, I think 
this would be good to go.

--
Added file: http://bugs.python.org/file31179/etree_3121.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

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!

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Eli Bendersky]

Eli Bendersky added the comment:

Thanks for the patch. I'll take a look.

--
assignee:  - eli.bendersky

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Robin Schreiber]

Robin Schreiber added the comment:

Patch updated to work with current 3.4 Branch version of elementtree.

--
keywords: +patch
Added file: http://bugs.python.org/file28311/_elementtree_pep3121-384_v1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:


--
nosy: +Arfrever

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Robin Schreiber]

Changes by Robin Schreiber robin.schrei...@me.com:


--
keywords: +pep3121 -patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Andrew Svetlov]

Changes by Andrew Svetlov andrew.svet...@gmail.com:


--
nosy: +asvetlov

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Robin Schreiber]

New submission from Robin Schreiber:

Changes proposed in PEP3121 and PEP384 have now been applied to the elementtree 
module!

--
components: Extension Modules
files: _elementtree_pep3121-384_v0.patch
keywords: patch
messages: 168217
nosy: Robin.Schreiber
priority: normal
severity: normal
status: open
title: PEP 3121, 384 refactoring applied to elementtree module
type: resource usage
versions: Python 3.4
Added file: http://bugs.python.org/file26805/_elementtree_pep3121-384_v0.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Robin Schreiber]

Changes by Robin Schreiber robin.schrei...@me.com:


--
nosy: +effbot

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15651] PEP 3121, 384 refactoring applied to elementtree module

[Antoine Pitrou]

Antoine Pitrou added the comment:

See issue15390 review comments :)

--
nosy: +eli.bendersky, pitrou
stage:  - patch review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15651
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com