sanitize decorators and callable parameters on entrypoint load #15932
Conversation
soamicharan
requested review from
cicdw,
desertaxle and
zzstoatzz
as code owners
CodSpeed Performance ReportMerging #15932 will not alter performanceComparing Summary
|
|
Thanks for the PR @soamicharan! Could you please open an GitHub issue with an MRE describing the bug you're fixing here? That will be very helpful for me to understand the issue and help me make sure that the solution fits in well with the surrounding code. |
Thanks for suggestion |
|
@desertaxle @zzstoatzz Please review and merge the MR |
hi @soamicharan - there are still tests failing here which we'll need to fix |
There was a problem hiding this comment.
I have a couple of comments in addition to the test failures that @zzstoatzz pointed out. Let us know if you have any questions!
| # Remove flow decorator callback hooks | ||
|
|
||
| def get_decorator_function_name(func_decorator): | ||
| """Retrieve decorator function name.""" | ||
| func_decorator = ( | ||
| func_decorator.func | ||
| if isinstance(func_decorator, ast.Call) | ||
| else func_decorator | ||
| ) | ||
| return ( | ||
| func_decorator.attr | ||
| if isinstance(func_decorator, ast.Attribute) | ||
| else func_decorator.id | ||
| ) | ||
|
|
||
| if func_def.decorator_list: | ||
| # Keep only prefect flow decorator only | ||
| func_def.decorator_list = [ | ||
| func_decorator | ||
| for func_decorator in func_def.decorator_list | ||
| if get_decorator_function_name(func_decorator) == "flow" | ||
| ] | ||
| exclude_keyword_args = ( | ||
| "on_failure", | ||
| "on_completion", | ||
| "on_cancellation", | ||
| "on_crashed", | ||
| "on_running", | ||
| ) | ||
|
|
||
| # Exclude callable type keyword arguments from flow decorator | ||
| for func_decorator in func_def.decorator_list: | ||
| if not hasattr(func_decorator, "keywords"): | ||
| continue | ||
|
|
||
| func_decorator.keywords = list( | ||
| filter( | ||
| lambda keyword_arg: keyword_arg.arg not in exclude_keyword_args, | ||
| func_decorator.keywords, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I'd prefer if this code were encapsulated in a function for readability. Also, the decorator_list is looped over twice, and the code could be condensed to a single loop instead of filtering out all non-flow decorators first.
| entrypoint = f"{tmp_path.joinpath('flow.py')}:flow_function" | ||
|
|
||
| result = safe_load_flow_from_entrypoint(entrypoint) | ||
| assert result is not None |
There was a problem hiding this comment.
Can you add an additional check validating the flow was loaded as expected? You should be able to call the flow and check the return value is (None, None) since the defaults are removed when loading.
|
This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment. |
During
prefect deploycommand execution, if python environment is not setup prefect load entrypoint using code parsing by ast module.Now this is done by
_sanitize_and_load_flowfunction which tries to sanitize code definition and try to load the flow.Now in flow decorator, if it used any other decorator or uses callback hooks
on_completion,on_runningetc. then it will throw ScriptError.So in this PR, try to sanitize the decorators and keywords used in flow decorator to try to load flow.
Checklist
<link to issue>"mint.json.