Update API #253
Update API #253
Conversation
|
Since this PR changes processing function API, maybe we should also remove/deprecate |
|
Maybe make all user-defined functions async? Also, maybe rename |
dff/pipeline/pipeline/actor.py
Outdated
| ctx.framework_states["actor"]["global_true_label"], | ||
| ctx.framework_states["actor"]["local_true_label"], | ||
| ctx.framework_states["actor"]["node_true_label"], | ||
| ) = await asyncio.gather(*[global_transitions_coro, local_transitions_coro, node_transitions_coro]) |
There was a problem hiding this comment.
I think we should discuss how we want the components to execute (parallel vs sequential).
Currently:
- pre-response/pre-transition functions depend on
pipeline.parallelize_processing - Script conditions are executed in parallel
- Labels are executed sequentially inside the groups
GLOBAL,LOCAL,NODE(two labels are executed sequentially if they are from the same group, but in parallel if they are from different groups) - Actor handlers - in parallel
- Service handlers - depends on
ServiceGroup.asynchronous - Extra handlers - depends on
asynchronousof the extra handlers
After discussing how we should handle the newly asynchronized functions we should record that information in the documentation (and also in #252; other than the asynchronous option, the guide should also mention other options such as the timeout option for extra handlers, @pseusys).
Apply suggestions by @RLKRo Co-authored-by: Roman Zlobin <[email protected]>
dff/pipeline/pipeline/actor.py
Outdated
| self, pipeline: Pipeline, ctx: Optional[Union[Context, dict, str]] = None, *args, **kwargs | ||
| ) -> Union[Context, dict, str]: | ||
| # context init | ||
| ctx = self._context_init(ctx, *args, **kwargs) | ||
| self._run_handlers(ctx, pipeline, ActorStage.CONTEXT_INIT, *args, **kwargs) | ||
| await self._run_handlers(ctx, pipeline, ActorStage.CONTEXT_INIT, *args, **kwargs) | ||
|
|
||
| # get previous node | ||
| ctx = self._get_previous_node(ctx, *args, **kwargs) |
There was a problem hiding this comment.
Don't we want to make transition functions asynchronous as well?
There was a problem hiding this comment.
We can leave such methods as get_previous_node and get_next_node synchronous for two reasons:
- all they do is get values from dictionaries, so making them asynchronous won't result in a performance improvement
- we would have to await them inside the actor call, i.e. use them like regular blocking functions
dff/pipeline/pipeline/actor.py
Outdated
| ctx.framework_states["actor"]["global_true_label"], | ||
| ctx.framework_states["actor"]["local_true_label"], | ||
| ctx.framework_states["actor"]["node_true_label"], | ||
| ) = await asyncio.gather(*[global_transitions_coro, local_transitions_coro, node_transitions_coro]) |
tests/script/core/test_tutorials.py
Outdated
| @@ -28,3 +30,11 @@ | |||
| def test_tutorials(tutorial_module_name: str): | |||
| tutorial_module = importlib.import_module(f"tutorials.{dot_path_to_addon}.{tutorial_module_name}") | |||
| check_happy_path(tutorial_module.pipeline, tutorial_module.happy_path) | |||
| random.seed(31415) | |||
There was a problem hiding this comment.
Why should we do it here?
I think it was more obvious the way it was.
Also, you haven't removed any seed calls from the tests themselves (in this PR) (if there were any).
There was a problem hiding this comment.
Could you deprecate overwrite_current_node_in_processing instead of removing it?
The deprecation warning should include info that this function is to be removed in 1.0.
Also, we should either make this function do nothing (which is what it currently does) or fix the bug preventing it from doing something.
The bug in question:
Node.model_validate(processed_node) -- doesn't work because here Node is pydantic.BaseModel instead of dff.script.core.script.Node.
Fixing this bug would prove difficult without the pipeline typing fixes from refactor/use_type_checking so I would suggest making this function do nothing except calling warnings.warn. In this case we should also change the documentation for this function (and wherever else it's mentioned) to say that this function doesn't and never worked and that it is not supposed to be used anymore and will be removed in 1.0.
|
ToDo:
|
dff/script/core/context.py
Outdated
| removed_in="1.0.0", | ||
| details="This method is deprecated and will be removed in future versions. " | ||
| "The earlier implementation was not functional due to typing issues.", | ||
| ) | ||
| def overwrite_current_node_in_processing(self, processed_node: Node): |
There was a problem hiding this comment.
I think before 1.0.0 we can safely remove methods without deprecation.
Special tests for parallel processing will be added instead.
I decided that the next release will be 1.0, so we can remove instead of deprecating.
As I said before it doesn't make much sense that they run in parallel while conditions in them run sequentially.
Also includes some doc and typing updates
# Conflicts: # dff/messengers/telegram/interface.py
Description
Implement asynchronous execution of actor handlers, processing and response functions
Checklist