Backport 3043 (remote register launch plan name) to v1.14 + 3024 #3045
Conversation
wild-endeavor
requested review from
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
Code Review Agent Run #5383bbActionable Suggestions - 2
Review Details
AI Code Review powered by |
wild-endeavor
changed the title
Changelist by BitoThis pull request implements the following key changes.
|
| @@ -1264,7 +1264,7 @@ def register_launch_plan( | |||
| :return: | |||
| """ | |||
| if serialization_settings is None: | |||
| _, _, _, module_file = extract_task_module(entity) | |||
| _, _, _, module_file = extract_task_module(entity.workflow) | |||
There was a problem hiding this comment.
Consider checking if entity.workflow exists before accessing it. The extract_task_module() function is being called with entity.workflow which may not exist on all entities.
Code suggestion
Check the AI-generated fix before applying
| _, _, _, module_file = extract_task_module(entity.workflow) | |
| if not hasattr(entity, 'workflow'): | |
| raise ValueError('Entity does not have a workflow attribute') | |
| _, _, _, module_file = extract_task_module(entity.workflow) |
Code Review Run #5383bb
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| def test_workflow_lhs(): | ||
| assert my_wf_example._lhs == "my_wf_example" | ||
|
|
There was a problem hiding this comment.
Consider adding more test cases to verify _lhs behavior in different scenarios like nested workflows or when workflow name is changed using decorators.
Code suggestion
Check the AI-generated fix before applying
| def test_workflow_lhs(): | |
| assert my_wf_example._lhs == "my_wf_example" | |
| def test_workflow_lhs(): | |
| assert my_wf_example._lhs == "my_wf_example" | |
| @workflow | |
| def nested_wf(): | |
| return my_wf_example(a=5) | |
| assert nested_wf._lhs == "nested_wf" | |
| renamed_wf = workflow(name="new_name")(my_wf_example) | |
| assert renamed_wf._lhs == "new_name" | |
Code Review Run #5383bb
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-v1.14 #3045 +/- ##
==================================================
+ Coverage 81.40% 93.22% +11.82%
==================================================
Files 200 23 -177
Lines 21012 1580 -19432
Branches 2707 0 -2707
==================================================
- Hits 17104 1473 -15631
+ Misses 3151 107 -3044
+ Partials 757 0 -757 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Code Review Agent Run #f4d751Actionable Suggestions - 0Review Details
AI Code Review powered by |
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
wild-endeavor
changed the title
Code Review Agent Run #d3c15aActionable Suggestions - 0Review Details
AI Code Review powered by |
Cherry-pick a06c4dae7
also https://github.com/flyteorg/flytekit/pull/3024/files
Summary by Bito
This PR backports changes from PR #3043 to version 1.14, enhancing workflow handling and remote execution capabilities. It implements proper _lhs attribute initialization in PythonFunctionWorkflow and improves remote launch plan registration. Additionally, it adds version constraint to apache-airflow-providers-google package in flytekit-airflow plugin, limiting it to versions below 12.0.0.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1