Py2.5 issue: decimal context manager misimplemented, misdesigned, and misdocumented
I would like to see the changes to the decimal module reverted for the Py2.5 release. Currently, the code in the decimal module implements the context manager as a separate class instead of incorporating it directly in decimal.Context. This makes the API unnecessarily complex and is not pretty compared to the code it was intended to replace. Worse still, the implementation saves a reference to the context instead of making a copy of it. Remember decimal.Context objects are mutable -- the current implementation does not fulfill its contract to restore the context to its original state at the conclusion of the with-statement. The right way to do it was presented in PEP343. The implementation was correct and the API was simple. Additionally: * The examples in WhatsNew don't work because the implementation uses a different method name to fetch to context (this is a shallow error except that the name in WhatsNew is better and we don't really want to have a new method for this). It doesn't bode well that none of the release candidate end users noticed this discrepancy -- it means they are not trying out the examples. * The implementation's doc string examples were not tested and don't work (this is a deep error). One reads: with decimal.getcontext() as ctx: ctx.prec += 2 s = ... return +s To get this to work with the current implementation, it should read with decimal.getcontext().copy().get_manager() as ctx: ctx.prec += 2 s = ... return +s This is horrid. Please either revert the patch or fix it to match PEP-343. Raymond
At 05:20 PM 8/29/2006 -0700, Raymond Hettinger wrote:
* The implementation's doc string examples were not tested and don't work (this is a deep error). One reads:
with decimal.getcontext() as ctx: ctx.prec += 2 s = ... return +s
To get this to work with the current implementation, it should read
with decimal.getcontext().copy().get_manager() as ctx: ctx.prec += 2 s = ... return +s
This is horrid. Please either revert the patch or fix it to match PEP-343.
Actually, as I read the code, that would be: with decimal.getcontext().get_manager() as ctx: Which is still horrible, but unfortunately Guido has already pronounced that __context__ must be removed from PEP 343, which is what caused this abomination to come about. The PEP currently offers the idea of a 'localcontext()' API that provides a nicer spelling, but it appears nobody implemented it.
Phillip J. Eby wrote:
At 05:20 PM 8/29/2006 -0700, Raymond Hettinger wrote:
* The implementation's doc string examples were not tested and don't work (this is a deep error). One reads:
with decimal.getcontext() as ctx: ctx.prec += 2 s = ... return +s
To get this to work with the current implementation, it should read
with decimal.getcontext().copy().get_manager() as ctx: ctx.prec += 2 s = ... return +s
This is horrid. Please either revert the patch or fix it to match PEP-343.
Actually, as I read the code, that would be:
with decimal.getcontext().get_manager() as ctx:
Given the current mis-implementation, the copy() step is absolutely necessary. Since context objects are mutable, the current context would never get it precision and flags restored. Try running the example and printing out the current context precision before and after the with-suite. You'll see that the context has changed (which defeats the whole purpose).
Which is still horrible, but unfortunately Guido has already pronounced that __context__ must be removed from PEP 343, which is what caused this abomination to come about.
The PEP currently offers the idea of a 'localcontext()' API that provides a nicer spelling, but it appears nobody implemented it.
Right. The PEP version was correct and desirable. But at this point, it is better to have nothing at all. Users can still write their own (a la example 8 in the PEP). I do not want the decimal API to be forever mucked-up; hence, we should revert the change. Since it is buggy, that makes the decision an easy one to swallow. Raymond
At 05:57 PM 8/29/2006 -0700, Raymond Hettinger wrote:
Phillip J. Eby wrote:
At 05:20 PM 8/29/2006 -0700, Raymond Hettinger wrote:
* The implementation's doc string examples were not tested and don't work (this is a deep error). One reads:
with decimal.getcontext() as ctx: ctx.prec += 2 s = ... return +s
To get this to work with the current implementation, it should read
with decimal.getcontext().copy().get_manager() as ctx: ctx.prec += 2 s = ... return +s
This is horrid. Please either revert the patch or fix it to match PEP-343.
Actually, as I read the code, that would be:
with decimal.getcontext().get_manager() as ctx:
Given the current mis-implementation, the copy() step is absolutely necessary. Since context objects are mutable, the current context would never get it precision and flags restored.
Try running the example and printing out the current context precision before and after the with-suite. You'll see that the context has changed (which defeats the whole purpose).
No need; now that you've explained the problem I see why the code is wrong. This is definitely a bug in the decimal module. It looks like the code is correct at first glance, but the .copy() definitely needs to be in the ContextManager class, not the get_manager() method. Yuck.
Raymond Hettinger wrote:
I would like to see the changes to the decimal module reverted for the Py2.5 release.
Currently, the code in the decimal module implements the context manager as a separate class instead of incorporating it directly in decimal.Context.
That should read "... as a separate class instantiated by a brand-new Context method instead of implementing it as a module level function as shown in section 9 of PEP343". The API in PEP343 is much cleaner. Also, it doesn't have the copy vs reference bug. Raymond
Raymond Hettinger wrote:
I would like to see the changes to the decimal module reverted for the Py2.5 release.
I believe you may be overreacting - I don't consider the current behaviour buggy and the module level API can be added later. That said, the docstring is definitely wrong, and I can't find any unit tests for the feature (I thought there were some in test_with, but I appear to be mistaken).
Currently, the code in the decimal module implements the context manager as a separate class instead of incorporating it directly in decimal.Context. This makes the API unnecessarily complex and is not pretty compared to the code it was intended to replace.
The removal of the __context__ method made it impossible to permit context objects to be used directly in with statements. Even when that was the case, the separate ContextManager class was necessary in order to correctly handle the restoration as context objects may be shared between threads or nested within a single thread [1]. The localcontext() function in PEP 343 does exactly the same thing - it merely uses a generator context instead of a direct implementation of __enter__ and __exit__. The current syntax (the get_manager() method) can easily be made prettier in the future by adding a sugar function at the module level: def localcontext(ctx=None): if ctx is None: ctx = getcontext() return ctx.get_manager()
Worse still, the implementation saves a reference to the context instead of making a copy of it. Remember decimal.Context objects are mutable -- the current implementation does not fulfill its contract to restore the context to its original state at the conclusion of the with-statement.
The implementation doesn't forget that. The context to restore is determined by calling getcontext() in the __enter__ method. The restored context has nothing to do with the context passed to the ContextManager constructor.
from decimal import getcontext() getcontext().prec 28 with getcontext().get_manager() as ctx: ... ctx.prec += 2 ... getcontext().prec 28
The only ways to break it are to call ContextManager directly with an existing context that someone else already has a reference to:
from decimal import getcontext() getcontext().prec 28 with ContextManager(getcontext()) as ctx: ... ctx.prec += 2 ... getcontext().prec 30
Or to deliberately reuse a ContextManager instance:
mgr = getcontext().get_manager() with mgr as ctx: ... ctx.prec += 2 ... print ctx.prec ... 32 with mgr as ctx: ... ctx.prec += 2 ... print ctx.prec ... 34
Cheers, Nick. [1] In fact, get_manager() is merely a new name for the old __context__ method. This name was suggested by Neal after I initially called the method context_manager(), but was never separately discussed on python-dev (the original discussion occurred in one of the massive PEP 343 threads). http://mail.python.org/pipermail/python-checkins/2006-May/052083.html -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
Nick Coghlan wrote:
Raymond Hettinger wrote:
I would like to see the changes to the decimal module reverted for the Py2.5 release.
I believe you may be overreacting - I don't consider the current behaviour buggy and the module level API can be added later.
My preference is to remove the method entirely and then implement the approach in PEP 343 by adding a module level "localcontext()" function in Py2.6. If you keep the method, then at least: * fix the docstring (and make it doctestable) * rename the method to .localcontext() * move the .copy() step to inside the contextmanager instead of its caller * update the WhatsNew example to match * add a unittest
Raymond Hettinger wrote:
The right way to do it was presented in PEP343. The implementation was correct and the API was simple.
Raymond's persuaded me that he's right on the API part at the very least. The current API was a mechanical replacement of the initial __context__ based API with a normal method, whereas I should have reverted back to the module-level localcontext() function from PEP343 and thrown the method on Context objects away entirely. I can fix it on the trunk (and add those missing tests!), but I'll need Anthony and/or Neal's permission to backport it and remove the get_manager() method from Python 2.5 before we get stuck with it forever. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
Nick Coghlan wrote:
Raymond Hettinger wrote:
The right way to do it was presented in PEP343. The implementation was correct and the API was simple.
Raymond's persuaded me that he's right on the API part at the very least. The current API was a mechanical replacement of the initial __context__ based API with a normal method, whereas I should have reverted back to the module-level localcontext() function from PEP343 and thrown the method on Context objects away entirely.
I can fix it on the trunk (and add those missing tests!), but I'll need Anthony and/or Neal's permission to backport it and remove the get_manager() method from Python 2.5 before we get stuck with it forever.
I committed this fix as 51664 on the trunk (although the docstrings are still example free because doctest doesn't understand __future__ statements). Anthony, Neal: I'd like to backport this change to the 2.5 maintenance branch. I realise it is an API change between the release candidate and the actual release, but this really is a small tweak to something nobody is actually using yet. If that's not acceptable, I'd like to go with Raymond's original option: rip it out entirely for 2.5 so we don't get stuck maintaining it for the rest of the 2.x series. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
The right way to do it was presented in PEP343. The implementation was correct and the API was simple.
Raymond's persuaded me that he's right on the API part at the very least. The current API was a mechanical replacement of the initial __context__ based API with a normal method, whereas I should have reverted back to the module-level localcontext() function from PEP343 and thrown the method on Context objects away entirely.
I can fix it on the trunk (and add those missing tests!), but I'll need Anthony and/or Neal's permission to backport it and remove the get_manager() method from Python 2.5 before we get stuck with it forever.
I committed this fix as 51664 on the trunk (although the docstrings are still example free because doctest doesn't understand __future__ statements).
Thanks for getting this done. Please make the following changes: * rename ContextManger to _ContextManger and remove it from the __all__ listing * move the copy() step from localcontext() to _ContextManager() * make the trivial updates the whatsnew25 example Once those nits are fixed, I recommend this patch be backported to the Py2.5 release. Raymond
On Thu, 31 Aug 2006, Nick Coghlan wrote: [...]
I committed this fix as 51664 on the trunk (although the docstrings are still example free because doctest doesn't understand __future__ statements). [...]
Assuming doctest doesn't try to parse the Python code when SKIP is specified, I guess this would solve that little problem: http://docs.python.org/dev/lib/doctest-options.html """ SKIP When specified, do not run the example at all. This can be useful in contexts where doctest examples serve as both documentation and test cases, and an example should be included for documentation purposes, but should not be checked. E.g., the example's output might be random; or the example might depend on resources which would be unavailable to the test driver. The SKIP flag can also be used for temporarily "commenting out" examples. ... Changed in version 2.5: Constant SKIP was added. """ John
John J Lee wrote:
On Thu, 31 Aug 2006, Nick Coghlan wrote: [...]
I committed this fix as 51664 on the trunk (although the docstrings are still example free because doctest doesn't understand __future__ statements). [...]
Assuming doctest doesn't try to parse the Python code when SKIP is specified, I guess this would solve that little problem:
http://docs.python.org/dev/lib/doctest-options.html
""" SKIP
A quick experiment suggests that using SKIP will solve the problem - fixing that can wait until 2.5.1 though. The localcontext() docstring does actually contain an example - it just isn't in a form that doctest will try to execute. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
Nick Coghlan wrote:
Raymond Hettinger wrote:
The right way to do it was presented in PEP343. The implementation was correct and the API was simple.
Raymond's persuaded me that he's right on the API part at the very least. The current API was a mechanical replacement of the initial __context__ based API with a normal method, whereas I should have reverted back to the module-level localcontext() function from PEP343 and thrown the method on Context objects away entirely.
I can fix it on the trunk (and add those missing tests!), but I'll need Anthony and/or Neal's permission to backport it and remove the get_manager() method from Python 2.5 before we get stuck with it forever.
Please go ahead and get the patch together for localcontext(). This should be an easy sell: * simple bugs can be fixed in Py2.5.1 but API mistakes are forever. * currently, all of the docs, docstrings, and whatsnew are incorrect. * the solution has already been worked-out in PEP343 -- it's nothing new. * nothing else, anywhere depends on this code -- it is as safe a change as we could hope for. Neal is tough, but he's not heartless ;-) Raymond
Raymond Hettinger wrote:
Please go ahead and get the patch together for localcontext(). This should be an easy sell:
* simple bugs can be fixed in Py2.5.1 but API mistakes are forever. * currently, all of the docs, docstrings, and whatsnew are incorrect. * the solution has already been worked-out in PEP343 -- it's nothing new. * nothing else, anywhere depends on this code -- it is as safe a change as we could hope for.
Neal is tough, but he's not heartless ;-)
I backported the changes and assigned the patch to Neal: http://www.python.org/sf/1550886 Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
participants (4)
-
John J Lee -
Nick Coghlan -
Phillip J. Eby -
Raymond Hettinger