Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python Future Support #2023

Merged
merged 6 commits into from Feb 26, 2019

Conversation

@SteVwonder
Copy link
Member

commented Feb 18, 2019

Adds a Future class that inherits from WrapperPimpl, and makes RPC a subclass of Future. f.rpc_create() and f.rpc_send() are now dropped in favor of f.rpc() and f.rpc().get(), respectively. The Future class wasn't designed to be directly instantiated, it is more of an abstract class currently.

This PR also switches the bindings over to the new-style cffi callbacks. They should be faster and avoid any issues with SELinux [1]. The unfortunate side-effect being that the default compiler on TOSS3 doesn't appreciate the auto-generated C code:

_core.c:2272:3: warning: missing initializer for field ‘reserved1’ of ‘struct _cffi_externpy_s’ [-Wmissing-field-initializers]
   { "continuation_callback", 0 };
   ^
_core.c:174:11: note: ‘reserved1’ declared here
     void *reserved1, *reserved2;

So I had to throw a -Wno-error=missing-field-initializers into the python bindings CFLAGS to get things working.

Design questions for the broader group (@trws especially):

  • Does the class inhertiance structure look reasonable? It feels kind of weird to me to have RPC.InnerWrapper inherit from Future.InnerWrapper, but it works 🤷‍♂ It also kind of bothers me that the Future class should not be directly instantiated (in the current design) and that there is nothing in the code preventing it (e.g., some ABCmeta magic). Feels like one or the other of those two limitations should go away.
  • What to do when .then times out? What happens in C? I assume ETIMEDOUT is set before the reactor calls the callback, but I don't see any examples in flux-core of anything checking for ETIMEDOUT inside a callback.
  • If .wait_for times out, do we want to raise an exception, if so, which one? Just a plain ol' EnvironmentError with errno set to ETIMEDOUT? Or do we want to go the way that socket and requests did, and have a Flux-specific timeout exception? I think what we do here is influenced by the answer to the previous question.
  • Is returning an ffi.buffer of the raw future payload the "right thing" to do as the default implemetion for Future.get()? I don't expect many, if any python users to use this interface, but for those that do, I was hoping to do something sane. AFAIU with an ffi buffer, users can easily get a bytes object or initialize a struct; which seems pretty useful.

Another step towards #1649
Closes #1778

@SteVwonder SteVwonder requested a review from trws Feb 18, 2019
@SteVwonder SteVwonder force-pushed the SteVwonder:python-futures branch from ded04ce to e340ac4 Feb 18, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

What to do when .then times out? What happens in C? I assume ETIMEDOUT is set before the reactor calls the callback, but I don't see any examples in flux-core of anything checking for ETIMEDOUT inside a callback.

Right, the future is fulfilled with an error (ETIMEDOUT), and the fact that it is fulfilled then triggers the callback. I don't think we've used it much, although unless the timeout condition was going to be special-cased, the error handling code might not explicitly test for it, e.g. something like

if (flux_future_get (f, NULL) < 0)
    log_err_exit ("thing");

would handle the error and just print "thing: Connection timed out".

@SteVwonder

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

Thanks @garlick! That makes sense. I like your suggestion of not special casing the timeout. We could just let the EnvironmentError get raised after a .get() and have errno set to ETIMEDOUT. Given that, I'm thinking we should also raise an EnvironementError on .wait_for timeouts (rather than some custom timeout exception) for consistency.

@trws

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

This looks like a good improvement to me, especially getting the callbacks cleaned up. As to the inheritence, it’s reasonable, but I think I’d prefer if we make it such that just the Future class has an internal PIMPL construction, and we can pass through the extra prefixes as part of the Future constructor. That way we have one less inheritence chain to worry about.

As to being something that shouldn’t be instantiated, it’s a bit like some of the other classes we have in that it can really only be created correctly by having a handle passed into it, or through a factory function basically. I’d at least make it so that if the constructor is invoked without a handle, or with None, it throws an exception.

For buffers, I think that’s a reasonable thing for us to use, but it feels leaky. I’m not sure I even want to allow a user to do that with the easy function though. Maybe have .get() return a bytes array and .get_raw() give out the buffer with a warning that the representation of raw isn’t guaranteed stable or something? In the common case I think we really want users to specify a get that “cooks” the result for them, but having the escape hatch there to get at the base makes sense. Actually, maybe even allowing the user to pass in a buffer to read into like get normally does would be a good way to help avoid having that done too much?

@SteVwonder SteVwonder force-pushed the SteVwonder:python-futures branch from e340ac4 to dce8c28 Feb 19, 2019
@SteVwonder SteVwonder force-pushed the SteVwonder:python-futures branch 3 times, most recently from b050a22 to bfd3355 Feb 20, 2019
@SteVwonder SteVwonder changed the title [WIP]: Python Future Support Python Future Support Feb 20, 2019
@SteVwonder

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

Thanks @trws for the feedback.

I think I’d prefer if we make it such that just the Future class has an internal PIMPL construction, and we can pass through the extra prefixes as part of the Future constructor. That way we have one less inheritence chain to worry about.

Makes a lot of sense. Done!

I’d at least make it so that if the constructor is invoked without a handle, or with None, it throws an exception.

👍 handle is now a required positional argument of the Future constructor

For buffers, I think that’s a reasonable thing for us to use, but it feels leaky.....

Your suggestions are good ones. Unfortunately, after I started trying to test the get method, I realized there is no way to get a size on the flux_future_get result, so producing a bytes string won't work. I suppose letting the user pass in a buffer would work, but the more I think about it, I don't think we really get a whole lot by exposing the raw bytes right now. Once we have a compelling, real-world use-case, I suggest we revisit this and decide how to handle it then. For now, I opted to just remove get from the Future class and instead let the subclasses handle implementing their own get.


I also threw in some more tests, some more methods (is_ready and wait_for), and some fixes for anonymous RPCs getting garbage collected before their callbacks run. I believe this PR is ready for a re-review.

@trws

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

@SteVwonder SteVwonder force-pushed the SteVwonder:python-futures branch from bfd3355 to 1708fb4 Feb 21, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Should there be some minimal documentation included with this PR for futures, or is that a topic for later on? An example or two of futures in action might be useful here, at minimum, maybe as test programs driven by sharness?

Edit: or rewrite some flux commands in python, like flux-ping, flux-event, or flux-logger?

@SteVwonder SteVwonder requested a review from chu11 Feb 21, 2019
@SteVwonder SteVwonder force-pushed the SteVwonder:python-futures branch 2 times, most recently from ecc45ab to c71733a Feb 21, 2019
@SteVwonder

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

Thanks @garlick! As we discussed face-to-face:

Should there be some minimal documentation included with this PR for futures

I opened a new issue (#2036) to track this.

An example or two of futures in action might be useful here, at minimum, maybe as test programs driven by sharness?

I think t0012-futures.py is what you are looking for.

or rewrite some flux commands in python, like flux-ping, flux-event, or flux-logger

I like this idea! As @grondo pointed out, we should solve #1993 first.

@@ -162,6 +162,12 @@ def build_argument_translation_list(self, fun_type):
def __call__(self, calling_object, *args_in):
calling_object.ffi.errno = 0
caller = calling_object.handle
if self.add_handle and caller is None:

This comment has been minimized.

Copy link
@chu11

chu11 Feb 21, 2019

Contributor

This change seems to be somewhat independent of this commit? I assume some bug was hit and you added it? Perhaps needs a comment in the commit or should be in a separate commit?

This comment has been minimized.

Copy link
@SteVwonder

SteVwonder Feb 22, 2019

Author Member

Oh. Good call. That was added when I had a destroy method for Future. But that method proved too tricky to handle consistency (too many corner cases), so I removed the method and then squashed the commit. This stuck around. I'll pull it out into its own commit (along with the relevant unit test, if there is one, if not, I'll make one)

This comment has been minimized.

Copy link
@SteVwonder

SteVwonder Feb 26, 2019

Author Member

@chu11: I pulled this out into it's own commit and added a unit test in t/python/wrapper.py in the same commit.

@SteVwonder SteVwonder force-pushed the SteVwonder:python-futures branch from c71733a to bccea59 Feb 26, 2019
SteVwonder added 6 commits Feb 17, 2019
statically define callbacks in `callbacks.h` with `extern "Python"`.

Include those definitions in the bindings via a new argument to
`make_bindings`: `--additional_headers`.

Add `-Wno-error` to python binding compilation to fatal compiler errors
from: `warning: missing initializer for field ‘reserved1’ of ‘struct
_cffi_externpy_s’ [-Wmissing-field-initializers]`
that is, `flux_handle.rpc()` and `flux_handle.rpc().get()`, respectively

Closes #1778
If the handle within a Wrapper is destroyed after the Wrapper's method
is already bound (and add_handle is set to True), then throw a
ValueError with a specific error message rather than allowing the more
general WrongNumArguments exception to bubble up.
@SteVwonder SteVwonder force-pushed the SteVwonder:python-futures branch from bccea59 to 873faaa Feb 26, 2019
@SteVwonder

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

Rebased and addressed @chu11's comment.

@codecov-io

This comment has been minimized.

Copy link

commented Feb 26, 2019

Codecov Report

Merging #2023 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2023      +/-   ##
==========================================
+ Coverage   80.59%   80.61%   +0.01%     
==========================================
  Files         180      180              
  Lines       28966    28966              
==========================================
+ Hits        23346    23351       +5     
+ Misses       5620     5615       -5
Impacted Files Coverage Δ
src/common/libflux/mrpc.c 87.74% <0%> (-1.19%) ⬇️
src/cmd/flux-event.c 78.57% <0%> (+0.59%) ⬆️
src/broker/ping.c 74.28% <0%> (+10%) ⬆️
@chu11 chu11 self-requested a review Feb 26, 2019
@chu11
chu11 approved these changes Feb 26, 2019
@chu11

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

LGTM. Dunno if @trws wanted to give it another look over.

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I think @trws is traveling, so let's put this in and we can always tweak it later if need be. Thanks!

@garlick garlick merged commit 880e050 into flux-framework:master Feb 26, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch Coverage not affected when comparing fdb8c6c...873faaa
Details
codecov/project 80.61% (+0.01%) compared to fdb8c6c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.