Cleanup Future API #292

1st1 · 2015-11-16T17:24:30Z

This PR cleans up Future class by factoring out _copy_state() and _set_result_unless_cancelled() into separate functions.

gvanrossum · 2015-11-16T18:43:55Z

asyncio/futures.py

@@ -155,6 +155,7 @@ def __init__(self, *, loop=None):
            self._source_traceback = traceback.extract_stack(sys._getframe(1))

    def _format_callbacks(self):
+        # Private method, do not rely on its existence.


Would it make sense to rename to __format_callbacks? Or are there subclasses that call it?

Yes, this is a good idea. Nothing is using this method besides the base Future class.

gvanrossum · 2015-11-16T18:47:30Z

Harmless, but suggesting some changes.

gvanrossum · 2015-11-16T21:46:22Z

Ping me when you want me to have another look. (GitHub's PR code review confuses me because it's so different from the tools I've been using for 10 years.)

gvanrossum · 2015-11-16T21:48:15Z

asyncio/futures.py

@@ -174,6 +175,7 @@ def format_cb(callback):
        return 'cb=[%s]' % cb

    def _repr_info(self):
+        # Private method, do not rely on its existence.


This and the private methods below can also be given a __ prefix right?

It's a bit more complicated. It's "private" in a sense that nobody outside of asyncio shouldn't rely on it. In asyncio, it's overloaded in Task, and in one other place--something that we can address when/if we decide to rewrite Future (and maybe Task) in C.

Oh, yeah. This is what we call "protected" -- subclasses can use/override. So just update the comment (presumably also for the others?).

Oh, yeah. This is what we call "protected" -- subclasses can use/override

Yeah, but I don't want for non-asyncio subclasses to rely on this method. "protected" means (at least in c++/java/etc) that it's OK for subclasses to override and/or use. Maybe, instead of "private" I should just put "do not use outside of asyncio" there?

Oh, sounds good.

1st1 · 2015-11-16T21:48:23Z

Ping me when you want me to have another look. (GitHub's PR code review confuses me because it's so different from the tools I've been using for 10 years.)

I've just pushed this commit that should address your comments: 16851f7

If it looks good, I'll squash all three commits in one and commit the diff by hand.

1st1 · 2015-11-16T22:03:50Z

Another commit -- 1st1@f63a339. I've fixed the terminology, and also added this comment to Task._step and Task._wakeup

1st1 · 2015-11-16T22:33:12Z

BTW, Future._loop is used quite frequently in tests and in asyncio code. Should I add Future.get_loop(), or we can do it later?

gvanrossum · 2015-11-16T22:40:19Z

Sorry for jerking you around, but looking at this version I think I would like to go back to single underscores. __private is just too much of a pain to deal with in the debugger. And I don't think the comments about not using these things outside asyncio are useful.

Also, what is the purpose of this PR again? Why can't your libuv Future class define _copy_state() and _set_result_unless_cancelled()?

gvanrossum · 2015-11-16T22:41:49Z

I think you've used up your review quota for the day. :-(

On Mon, Nov 16, 2015 at 2:33 PM, Yury Selivanov notifications@github.com
wrote:

BTW, Future._loop is used quite frequently in tests and in asyncio code.
Should I add Future.get_loop(), or we can do it later?

—
Reply to this email directly or view it on GitHub
#292 (comment).

--Guido van Rossum (python.org/~guido)

This reverts commit 4ad1fb7.

1st1 · 2015-11-16T22:46:34Z

Also, what is the purpose of this PR again? Why can't your libuv Future class define _copy_state() and _set_result_unless_cancelled()?

I don't want to reimplement them. There is no need to those methods to be a part of the Future API. I hope that we'll accelerate Futures and Tasks in 3.5.x, and I want it to be as painless as possible.

gvanrossum · 2015-11-16T22:50:19Z

Please describe your proposed strategy in more detail. What's going to be in the accelerator module, what remains in Python, and what's going to be in the libuv-specific module?

1st1 · 2015-11-16T23:08:22Z

Please describe your proposed strategy in more detail. What's going to be
in the accelerator module, what remains in Python, and what's going to be
in the libuv-specific module?

It would be great if we can merge this PR before 3.5.1, so that I can implement the Future in C without implementing not strictly necessary _copy_state() and _set_result_unless_cancelled(). The less C code I have, the easier it would be to thoroughly review and test it. There is no real reason for those methods to be defined in the Future class.

Sometime after 3.5.1, I plan to have a stable C Future implementation as part of the uvloop project. To make things simpler, I hope that we'll be able to integrate that implementation directly into asyncio for everybody's benefit. Ideally, futures.py can become something like this:

try:
    from ._futures import Future
except ImportError:
    class Future:
        ...

def _copy_future_state(fut, other_fut):
    ...

def _set_result_unless_cancelled(fut, result):
    ...

def _chain_future(source, destination):
    ...

# etc

I still have to play with implementing asyncio.Task in C. If it gives another visible performance boost, we might want to do that too. Or we can simply derive asyncio.Task from Future implemented in C. But this is something quite remote to plan for right now.

gvanrossum · 2015-11-16T23:24:20Z

Well it all sound way too rushed just so you can show off some benchmark numbers. Sure, the changes are very minor and very unlikely to break anything, but I've lost confidence that this is the end. E.g. if you're so keen on having the minimal amount of C code, wouldn't you want to avoid having to write _repr_info() and _format_callbacks() in C too? Honestly, I don't mind if you commit the changes for _copy_state and _set_result_unless_cancelled() but I don't really expect that this will make the 3.5.2 changes any easier to digest.

1st1 · 2015-11-16T23:52:41Z

Well it all sound way too rushed just so you can show off some benchmark
numbers.

Benchmark numbers are great, but the reason why I'm investing a ton of time in asyncio is because we're building a database (based on PostgreSQL) that's built with it. Python allows us to iterate faster. I'm trying to improve things where it's possible, and Future object is one of the core building blocks.

Sure, the changes are very minor and very unlikely to break
anything, but I've lost confidence that this is the end.

I understand. Things that I try to fix/improve are only visible when you're trying to implement a compatible loop/interfaces from the outside of asyncio (most of my recent PR were related to that).

E.g. if you're so
keen on having the minimal amount of C code, wouldn't you want to avoid
having to write _repr_info() and _format_callbacks() in C too?

Those are only used (and usable) in Future.__repr__. Right now, I'm not even implementing __repr__. Same thing about _schedule_callbacks -- there is little reason to call it for anybody. In contrast, helpers like _copy_future_state or _set_result_unless_cancelled can be handy, and thus may be used by someone, therefore I think it's better to keep those things separate.

gvanrossum · 2015-11-17T00:04:56Z

Well, but an acceptable C Future should have a compatible `__repr__` (or at least one providing the same level of insight and details). Anyway, go ahead and commit the refactor of `_copy_state()` and `_set_result_unless_cancelled()`, but I'm still skeptical about the road that lies ahead. ;-)

Makes Future._set_result_unless_cancelled and Future._copy_state standalone functions.

1st1 · 2015-11-17T17:15:28Z

Well, but an acceptable C Future should have a compatible __repr__ (or at
least one providing the same level of insight and details).

Absolutely.

Anyway, go ahead and commit the refactor of _copy_state() and
_set_result_unless_cancelled(), but I'm still skeptical about the road
that lies ahead. ;-)

Merged (without "private" comments).

gvanrossum · 2015-11-17T17:53:18Z

Thanks for your understanding! And good luck with libuv -- it sounds really
cool (and it was one of the things PEP 3156 explicitly wanted to encourage).

On Tue, Nov 17, 2015 at 9:15 AM, Yury Selivanov notifications@github.com
wrote:

Well, but an acceptable C Future should have a compatible repr (or at
least one providing the same level of insight and details).

Absolutely.

Anyway, go ahead and commit the refactor of _copy_state() and
_set_result_unless_cancelled(), but I'm still skeptical about the road
that lies ahead. ;-)

Merged (without "private" comments).

—
Reply to this email directly or view it on GitHub
#292 (comment).

--Guido van Rossum (python.org/~guido)

See python/asyncio#292 for details. --HG-- branch : 3.4

1st1 added 2 commits November 16, 2015 12:14

Factor out Future._copy_state() into futures._apply_future_state

4e657ec

Factor out Future._set_result_unless_cancelled into a separate function

6ca81e9

1st1 changed the title ~~Public future~~ Cleanup Future API Nov 16, 2015

gvanrossum reviewed Nov 16, 2015
View reviewed changes

Address Guido's feedback

16851f7

gvanrossum reviewed Nov 16, 2015
View reviewed changes

Fix the terminology.

f63a339

Rename Task._step -> Task.__step; Task._wakeup -> Task.__wakeup

4ad1fb7

Revert "Rename Task._step -> Task.__step; Task._wakeup -> Task.__wakeup"

b42abbb

This reverts commit 4ad1fb7.

1st1 added a commit that referenced this pull request Nov 17, 2015

Cleanup Future API (PR #292)

4a7100b

Makes Future._set_result_unless_cancelled and Future._copy_state standalone functions.

1st1 closed this Nov 17, 2015

akheron pushed a commit to akheron/cpython that referenced this pull request Nov 18, 2015

asyncio: Cleanup Future API

99ffdeb

See python/asyncio#292 for details. --HG-- branch : 3.4

Uh oh!

Cleanup Future API #292

Cleanup Future API #292

Uh oh!

Conversation

1st1 commented Nov 16, 2015

Uh oh!

gvanrossum Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

1st1 Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Nov 16, 2015

Uh oh!

gvanrossum commented Nov 16, 2015

Uh oh!

gvanrossum Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

1st1 Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

gvanrossum Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

1st1 Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

gvanrossum Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

1st1 commented Nov 16, 2015

Uh oh!

1st1 commented Nov 16, 2015

Uh oh!

1st1 commented Nov 16, 2015

Uh oh!

gvanrossum commented Nov 16, 2015

Uh oh!

gvanrossum commented Nov 16, 2015

Uh oh!

1st1 commented Nov 16, 2015

Uh oh!

gvanrossum commented Nov 16, 2015 via email

Uh oh!

1st1 commented Nov 16, 2015

Uh oh!

gvanrossum commented Nov 16, 2015 via email

Uh oh!

1st1 commented Nov 16, 2015

Uh oh!

gvanrossum commented Nov 17, 2015 via email

Uh oh!

1st1 commented Nov 17, 2015

Uh oh!

gvanrossum commented Nov 17, 2015

Uh oh!

Uh oh!