classification
Title: test.support method for dual-testing accelerated code (fixes test_exceptions and other's pickle testing)
Type: enhancement Stage: needs patch
Components: Library (Lib) Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: alexandre.vassalotti, belopolsky, benjamin.peterson, ezio.melotti, mark.dickinson, pitrou, r.david.murray
Priority: normal Keywords: patch

Created on 2010-06-28 16:18 by belopolsky, last changed 2014-09-29 14:23 by belopolsky.

Files
File name Uploaded Description Edit
test_exceptions.diff belopolsky, 2010-06-28 16:18
issue9104.diff belopolsky, 2010-06-28 19:32
Messages (15)
msg108840 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-28 16:18
This is probably one example of many where pickling is only tested with _pickle module if it is available rather than separately with C and Python implementation.  I am attaching a patch which implements one possible testing strategy.  If this is acceptable, I think the machinery of retrieving multiple module implementations should be moved to test.support and reused in other test modules.
msg108843 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-06-28 17:08
It looks good on the principle. As you say, factoring out this kind of machinery in test.support is probably the way to go.
msg108854 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-28 19:30
In the new patch, issue9104.diff, I factored out import machinery into test.support.import_module_implementations and added it to a a couple of other test modules.  I did not attempt to improve all pickle tests.

The availability of import_module_implementations raises an interesting question of what to do in cases like heapq where one can potentially test 4 combinations of heapq/_heapq with pickle/_pickle.  Given that currently pickling of heapq objects is either not supported or not tested, we don't need to worry about it, but as parallel optimized/pure python module implementations proliferate, we may face exponential explosion of potential test cases.
msg108860 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2010-06-28 20:36
The code for import_module_implementations seems a bit fragile. Prefixing the module name with an underscore might not always yield the name of the optimized implementation.  An explicit dictionary to map the Python and C implementations may be a better approach.

Otherwise, the code looks good. Feel free to commit it.
msg108881 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-06-28 23:43
> An explicit dictionary to map the Python and C implementations may be a 
> better approach.

+1.
msg108941 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-29 19:49
> An explicit dictionary to map the Python and C implementations may be a 
> better approach.


Do you mean a global 

optimized_module = {'pickle': '_pickle'}

in test/support.py?


I don't think I like this idea.  Even with an explicit dictionary like that,  support.import_module_implementations() will not support all possible ways that alternative implementations of a given module may be provided in cpython.  It works for pickle/_pickle and heapq/_heapq, but won't work for io/_io/_pyio.  I would like import_module_implementations() to promote uniformity in the way parallel pure/native modules are designed.  Test suites for modules that want more flexibility can always call import_fresh_module() directly.
msg109026 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-01 01:02
I would like to commit this as written.  If a better mechanism for associating native implementation with a pure python module is found, it can easily be added in the future.  Any objections?  The patch only adds more test cases, no code is changed.
msg109032 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2010-07-01 05:47
> It works for pickle/_pickle and heapq/_heapq, but won't work for io/_io/_pyio.

You can make the dictionary values as lists for the 'blocked' argument for import_fresh_module(). That would work.

And, can you add documentation for import_module_implementations()? A description how it finds the optimized implementation of a module would be helpful too.
msg109053 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-01 14:28
> You can make the dictionary values as lists for the 'blocked'
> argument for import_fresh_module(). That would work [for io].

I don't understand how having multiple modules in the blocked list will help in io case.  io.py will simply not work if _io is blocked.  It does not use "define in Python, override with native if available" strategy.  Instead, io.py and _pyio.py are independent complete implementations.

I don't like that approach because it makes pure python code hard to discover.  In the recent discussions, some people were in fact surprised to learn that pure python io implementation is still available.  The io/_pyio approach also prevents io.py from bring used by alternative python implementations unmodified.


What I can do, is to add an optional "blocked" argument import_module_implementations() along the lines of

def import_module_implementations(name, blocked=None):
    if blocked is None:
        blocked = ('_' + name,)
    ...

This will not solve the io issue, but will add some flexibility.

Note that import_module_implementations() as designed is not very useful for tests of the named module itself.  In that case you need the implementations individually rather than as a list.  This is mostly useful in situations like pickle where other modules are tested for interoperability with alternative pickle implementations.

Of course, I should document the mechanism once we agree on what it should be. :-)  (I did not know that test.support had a reST document, but will update it for this patch.)
msg109059 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-01 16:18
> > You can make the dictionary values as lists for the 'blocked'
> > argument for import_fresh_module(). That would work [for io].
> 
> I don't understand how having multiple modules in the blocked list
> will help in io case.  io.py will simply not work if _io is blocked. 

Which you avoid by giving an empty list of blocked modules, using
Alexandre's suggestion.

> I don't like that approach because it makes pure python code hard to
> discover.

Ok, but this code exists and it would be much better if it were
supported.

> The io/_pyio approach also prevents io.py from bring used by
> alternative python implementations unmodified.

It would be foolish to use it unmodified anyway, unless you like
low-speed I/O (and a JIT isn't a magic bullet).

The reason this was done like this is that the io module is imported at
startup: we want to avoid unnecessary parsing of extraneous code (and
unnecessary importing additional dependencies), and we also want to
reduce opportunities for failing to initialize the standard I/O streams
(especially stderr...).

> This will not solve the io issue, but will add some flexibility.

Which is pointless unless such flexibility is needed by someone.
msg109060 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-07-01 16:37
Please don't update the reST document.  The support module should never have been documented outside the module itself, because now we are pretty much committed to otherwise needless backward compatibility for the stuff that is documented.
msg109062 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-01 17:52
On Thu, Jul 1, 2010 at 12:18 PM, Antoine Pitrou <report@bugs.python.org> wrote:
..
>> I don't understand how having multiple modules in the blocked list
>> will help in io case.  io.py will simply not work if _io is blocked.
>
> Which you avoid by giving an empty list of blocked modules, using
> Alexandre's suggestion.
>

Yes, you can make import_module_implementations('io') return [io] this way, but what the user is likely to expect would be [io, _pyio].


I understand that there are reasons to keep io the way it is, but most of the other modules should probably follow pickle approach.  In any case, testing alternative io implementations is easy.  No import block trickery is needed - just import io and _pyio separately.

I just don't like the idea of having test.support know details about other modules and would like to have a clear use case before doing anything more sophisticated than blocking '_' + name.

If we end up with a central registry of native optimizations, it should probably not be in test.support.
msg109082 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-07-01 21:59
If not in test.support, then where?  Unless there's some reason for user code or other implementations (or stdlib code itself) to access that map, it seems to me that test.support is exactly where it belongs.

Generic test support (that isn't specific to Python/stdlib testing) should go in unittest (and yes there is stuff currently in test.support that should move to unittest after suitable improvement and discussion...there's an issue about that.)
msg184150 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-03-14 08:31
> there's an issue about that.

That would be #8273.  See also #17037 and PEP 399.
msg227710 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-09-27 18:37
Looks like there is still some disagreement about the implementation here, so this isn't actually ready to commit yet?
History
Date User Action Args
2014-09-29 14:23:39belopolskysetassignee: belopolsky ->
2014-09-27 18:37:26r.david.murraysetversions: + Python 3.5, - Python 3.2
title: test_exceptions does not test pickling with pickle.py -> test.support method for dual-testing accelerated code (fixes test_exceptions and other's pickle testing)
messages: + msg227710

resolution: accepted ->
stage: commit review -> needs patch
2013-03-14 08:31:08ezio.melottisetnosy: + ezio.melotti
messages: + msg184150
2010-07-01 21:59:40r.david.murraysetmessages: + msg109082
2010-07-01 17:52:57belopolskysetmessages: + msg109062
2010-07-01 16:37:32r.david.murraysetnosy: + r.david.murray
messages: + msg109060
2010-07-01 16:18:34pitrousetmessages: + msg109059
2010-07-01 14:28:55belopolskysetmessages: + msg109053
2010-07-01 05:47:40alexandre.vassalottisetmessages: + msg109032
2010-07-01 01:02:55belopolskysetassignee: belopolsky
resolution: accepted
stage: patch review -> commit review
2010-07-01 01:02:33belopolskysetnosy: + mark.dickinson
messages: + msg109026
2010-06-29 19:49:41belopolskysetmessages: + msg108941
2010-06-28 23:43:48pitrousetmessages: + msg108881
2010-06-28 20:36:52alexandre.vassalottisetmessages: + msg108860
2010-06-28 19:32:54belopolskysetfiles: + issue9104.diff
2010-06-28 19:31:44belopolskysetfiles: - issue9104.diff
2010-06-28 19:30:03belopolskysetfiles: + issue9104.diff

messages: + msg108854
2010-06-28 17:08:18pitrousetmessages: + msg108843
2010-06-28 16:58:30belopolskysetnosy: + benjamin.peterson
2010-06-28 16:18:10belopolskycreate