Fixes __context for classed based actions #449
Conversation
1. Python name mangles __variables on methods. 2. We need to account for that for things to work and inject it properly. We don't modify the inputs dict since we don't want to change anything, so we shallow copy it, and change the single key value. Updates docs and adds example to show people.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 5a36d46 in 59 seconds
More details
- Looked at
342lines of code in6files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. burr/core/application.py:146
- Draft comment:
The current implementation of_remap_context_variableonly checks for parameters ending with__context. Consider checking for the presence of__contextin the parameter name to handle different mangling scenarios more robustly. - Reason this comment was not posted:
Confidence changes required:50%
The function _remap_context_variable is designed to handle the mangling of the __context variable in class-based actions. However, the current implementation only checks for parameters ending with __context, which might not be sufficient if the mangled name is different. A more robust approach would be to check for the presence of __context in the parameter name, regardless of its position.
Workflow ID: wflow_QaOryZCeOL5RlgpY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
A preview of is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/449 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
| @@ -103,6 +103,31 @@ def _adjust_single_step_output( | |||
| _raise_fn_return_validation_error(output, action_name) | |||
|
|
|||
|
|
|||
| def _remap_context_variable(run_method: Callable, inputs: Dict[str, Any]) -> dict: | |||
There was a problem hiding this comment.
We should do this with any __ variable... __tracer will hit the same problem. Could hardcode this, but we should consider being more aware of the context.
| @@ -121,6 +146,9 @@ def _run_function(function: Function, state: State, inputs: Dict[str, Any], name | |||
| ) | |||
| state_to_use = state.subset(*function.reads) | |||
| function.validate_inputs(inputs) | |||
| if "__context" in inputs: | |||
| # potentially need to remap the __context variable | |||
| inputs = _remap_context_variable(function.run, inputs) | |||
There was a problem hiding this comment.
I don't think remapping the context variable should be aware of the signature of the run function. Who knows how it would be created...
Rather, let's just hardcode the name-mangled version and unmangle them? Or maybe do it upstream?
|
will handle |
We don't modify the inputs dict since we don't want to change anything, so we shallow copy it, and change the single key value.
Updates docs and adds example to show people.
Changes
How I tested this
Notes
Checklist
Important
Fixes handling of
__contextin class-based actions by remapping mangled variable names and updates documentation and tests accordingly._remap_context_variable()inapplication.pyto handle name mangling of__contextin class-based actions._run_function()inapplication.pyto use_remap_context_variable()when__contextis in inputs.actions.rstto include examples of using__contextin class-based actions.application_classbased.pyas an example of class-based actions using__context._remap_context_variable()intest_application.pyto verify correct remapping of__context.test_application.pyto test class-based action behavior with__context.README.mdinhello-world-counterto referenceapplication_classbased.pyfor class-based action examples.This description was created by
for 5a36d46. It will automatically update as commits are pushed.