-
Notifications
You must be signed in to change notification settings - Fork 18
Adding dispatch decorator that simplifies common dispatch patterns #69
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
base: main
Are you sure you want to change the base?
Conversation
|
The test runs are not relevant to this PR. We might want something like this soon on a consulting project. @mdickinson do you mind taking a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much prefer to see two separate decorators here. We might even drop the call option altogether: did you have specific use-cases in mind for this? If not, can we wait until those use-cases exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much prefer to see two separate decorators here.
No problem. Now to name them...
We might even drop the call option altogether: did you have specific use-cases in mind for this?
I had traits.trait_notifiers.ui_dispatch in mind for call. I would be happy to drop it if we had a UIExecutor that dispatched to a UI thread. (That can get pretty hairy, though.)
|
I agree this looks potentially useful. I'm looking for actual places I'd use it, and haven't found any yet. I'll keep you posted. I'd strongly prefer to see two separate decorators rather than one multi-purpose decorator with a horrible signature. :-) What happens if you try to combine the |
|
Originally I had planned on migrating the 'dispatch' argument of the Here is an example of where I would use this. I want the decorators to play nicely, and I forgot about the traits call signature issue. I want that to work before merging this. I'll need more tests, but they should not live in encore. |
|
So in the example you linked to, you wouldn't want to decorate the I did find examples where I'd use this, and they look similar: I didn't find any cases where I'd want to apply the |
|
Image you do not have the option to turn off asynchronous updates in that example. In that case, you want all changes to a given trait to trigger a state update that goes through the asynchronizer. It would look something like... @on_trait_change('blur_level, image', post_init=True)
@dispatch("_asynchronizer")
def _recalculate_blurred_image(self):
""" Blur the image asynchronously """
self.blurred_image = blur_image(self.image, self.blur_level)
ui_dispatch(self.plot_data.set_data, "blurred_image", self.blurred_image) |
|
Hmm. That example makes me a bit nervous, because rather than using the new values of |
|
That is true. This is a bad idea in general, and for that reason, we may not want the decorators to play nicely together (with a good set of documentation explaining why). So, we're at a point where we're trading off a call to an executor-like object for a decorator that references that object by name. The latter is more declarative, but doesn't save much work. I'll tinker around with this more later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is meant to be sugar, then can't dispatcher handle either a string, or callable or real dispatcher instance? If you want this to be explicit, then it seems more consistent to have dispatcher=None, dispatcher_trait=None, callback=None.
This decorator is meant to eliminate some of the boilerplate associated with effectively using executors and work schedulers like here. The decorator also handles dispatch with callables (e.g.
ui_dispatch,deferred_call).Feedback on the functionality and API is much appreciated (@sjagoe, @mdickinson, @prabhuramachandran). If this is determined to be merge-worthy, I'll write up some documentation and examples.