callbacks: introduce scoped callbacks #1460
Conversation
skshetry
force-pushed
the
scoped-callback
branch
2 times, most recently
from
85a0575 to
b187043
Compare
1. This PR introduces a `close()` API that gives an opportunity for callbacks to close themselves, either on error or on completions. We use this to extend `Callback`s into a context-manager by using that `close()` api on `__exit__()`. 2. This PR also introduces `branched()` API that returns an instance of a callback. Since the callback is a context-manager, this gives an opportunity to close child callbacks after they complete. This is very similar to `branch()` API, but unlike `branch`, it should not mutate passed `kwargs`, and always return an instance of a callback. The default return is _DEFAULT_CALLBACK or NoopCallback if left unimplemented. For backwards compatibility, this will still call `branch()` API, try to read from the kwargs it has set, pop the `callback` kwarg and return it. But anyone implementing `Callback` does not need to implement `branch` API and should prefer `branched()` instead from now on, and the older API should be considered deprecated. Closes fsspec#1075.
skshetry
force-pushed
the
scoped-callback
branch
from
35409f1 to
8f78dc3
Compare
|
ping @martindurant |
|
Could we please have some tests that call the new API? I see you made some changes in the batch operations, and the existing tests pass, so that's good. |
| @@ -568,8 +568,8 @@ async def _put( | |||
| coros = [] | |||
| callback.set_size(len(file_pairs)) | |||
| for lfile, rfile in file_pairs: | |||
| callback.branch(lfile, rfile, kwargs) | |||
| coros.append(self._put_file(lfile, rfile, **kwargs)) | |||
| put_file = callback.branch_coro(self._put_file) | |||
There was a problem hiding this comment.
I don't like adding branch_coro but I am not sure how to support scoped/structured callbacks in async without one either, since we are working task/futures.
There was a problem hiding this comment.
Yeah, this is not well explorer territory. I would have thought that all calls into the callbacks machinery can by sync, since they will be fast and so safe to call from sync or async contexts.
I have added the tests. |
|
@efiop , any thoughts on this? |
| def close(self): | ||
| """Close callback.""" | ||
|
|
||
| def branched(self, path_1, path_2, kwargs): |
There was a problem hiding this comment.
Here, I matched the function signature with branch, but I am open to any suggestions here (if there should be kwargs or **kwargs or if it should be there at all).
There was a problem hiding this comment.
I have no better suggestion.
There was a problem hiding this comment.
I am trying to think of what could be the use case of kwargs. So far, it feels like an unnecessary required argument to pass.
I think following will be an example of how someone will use a branched callback.
with callback.branched(path_1, path_2, {}) as child:
passThe kwargs feel unnecessary to me. Either it should be kwargs=None by default or should be removed. I am leaning towards removing it though (can be added if we get some requests).
This PR introduces a
close()API that gives an opportunity for callbacks to close themselves, either on error or on completions. We use this to extendCallbacks into a context-manager by using thatclose()api on__exit__().This PR also introduces
branched()API that returns an instance of a callback. Since the callback is a context-manager, this gives an opportunity to close child callbacks after they complete. This is very similar tobranch()API, but unlikebranch, it should not mutate passedkwargs, and always return an instance of a callback. The default return is _DEFAULT_CALLBACK or NoopCallback if left unimplemented.For backwards compatibility, this will still call
branch()API, try to read from the kwargs it has set, pop thecallbackkwarg and return it. But anyone implementingCallbackdoes not need to implementbranchAPI and should preferbranched()instead from now on, and the older API should be considered deprecated.Closes #1075.