On May 22, 2010, at 8:47 PM, Brian Quinlan wrote: > Jesse, the designated pronouncer for this PEP, has decided to keep discussion > open for a few more days. > > So fire away!
As you wish! The PEP should be consistent in its usage of terminology about callables. It alternately calls them "callables", "functions", and "functions or methods". It would be nice to clean this up and be consistent about what can be called where. I personally like "callables". The execution context of callable code is not made clear. Implicitly, submit() or map() would run the code in threads or processes as defined by the executor, but that's not spelled out clearly. More relevant to my own interests, the execution context of the callables passed to add_done_callback and remove_done_callback is left almost completely to the imagination. If I'm reading the sample implementation correctly, <http://code.google.com/p/pythonfutures/source/browse/branches/feedback/python3/futures/process.py#241>, it looks like in the multiprocessing implementation, the done callbacks are invoked in a random local thread. The fact that they are passed the future itself *sort* of implies that this is the case, but the multiprocessing module plays fast and loose with object identity all over the place, so it would be good to be explicit and say that it's *not* a pickled copy of the future sitting in some arbitrary process (or even on some arbitrary machine). This is really minor, I know, but why does it say "NOTE: This method can be used to create adapters from Futures to Twisted Deferreds"? First of all, what's the deal with "NOTE"; it's the only "NOTE" in the whole PEP, and it doesn't seem to add anything. This sentence would read exactly the same if that word were deleted. Without more clarity on the required execution context of the callbacks, this claim might not actually be true anyway; Deferred callbacks can only be invoked in the main reactor thread in Twisted. But even if it is perfectly possible, why leave so much of the adapter implementation up to the imagination? If it's important enough to mention, why not have a reference to such an adapter in the reference Futures implementation, since it *should* be fairly trivial to write? The fact that add_done_callback is implemented using a set is weird, since it means you can't add the same callback more than once. The set implementation also means that the callbacks get called in a semi-random order, potentially creating even _more_ hard-to-debug order of execution issues than you'd normally have with futures. And I think that this documentation will be unclear to a lot of novice developers: many people have trouble with the idea that "a = Foo(); b = Foo(); a.bar_method != b.bar_method", but "import foo_module; foo_module.bar_function == foo_module.bar_function". It's also weird that you can remove callbacks - what's the use case? Deferreds have no callback-removal mechanism and nobody has ever complained of the need for one, as far as I know. (But lots of people do add the same callback multiple times.) I suggest having have add_done_callback, implementing it with a list so that callbacks are always invoked in the order that they're added, and getting rid of remove_done_callback. futures._base.Executor isn't exposed publicly, but it needs to be. The PEP kinda makes it sound like it is ("Executor is an abstract class..."). Plus, A third party library wanting to implement an executor of its own shouldn't have to copy and paste the implementation of Executor.map. One minor suggestion on the "internal future methods" bit - something I wish we'd done with Deferreds was to put 'callback()' and 'addCallbacks()' on separate objects, so that it was very explicit whether you were on the emitting side of a Deferred or the consuming side. That seems to be the case with these internal methods - they are not so much "internal" as they are for the producer of the Future (whether a unit test or executor) so you might want to put them on a different object that it's easy for the thing creating a Future() to get at but hard for any subsequent application code to fiddle with by accident. Off the top of my head, I suggest naming it "Invoker()". A good way to do this would be to have an Invoker class which can't be instantiated (raises an exception from __init__ or somesuch), then a Future.create() method which returns an Invoker, which itself has a '.future' attribute. Finally, why isn't this just a module on PyPI? It doesn't seem like there's any particular benefit to making this a stdlib module and going through the whole PEP process - except maybe to prompt feedback like this :). Issues like the ones I'm bringing up could be fixed pretty straightforwardly if it were just a matter of filing a bug on a small package, but fixing a stdlib module is a major undertaking.
_______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
