Add support cache for dynamic spec by popojk · Pull Request #3209 · flyteorg/flytekit

popojk · 2025-03-25T05:47:24Z

Tracking issue

Related to #5543

Why are the changes needed?

We need to pass execution mode into TaskTemplate Metadata for Flyte Propeller to tell whether the node is a Dynamic node.

What changes were proposed in this pull request?

Pass execution mode into TaskTemplate Metadata while registering workflows.

Check all the applicable boxes

All new and existing tests passed.
All commits are signed-off.

Related PRs

#6372

Summary by Bito

This PR adds execution_mode support to determine task behavior in dynamic caching scenarios, implementing critical improvements for caching in dynamic specifications. It enhances core modules with properties and conversion functions, ensures default values when execution_mode is not provided, and refines type logic to manage circular dependencies while aligning production code with tests for consolidated dynamic workflow support.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

Signed-off-by: Alex Wu <c.alexwu@gmail.com>

welcome · 2025-03-25T05:47:27Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

flyte-bot · 2025-03-25T05:47:34Z

Code Review Agent Run #0d112d

Actionable Suggestions - 4

tests/flytekit/common/parameterizers.py - 1
- Missing comma in product function parameters · Line 142-142
flytekit/core/base_task.py - 1
- Undefined name in type annotation · Line 143-143
flytekit/core/python_function_task.py - 1
- Missing fallback for unknown enum values · Line 144-144
flytekit/models/task.py - 1
- Missing property getter for execution_mode attribute · Line 228-231

Review Details

Files reviewed - 7 · Commit Range: 58ddff9..0bce2cf
- flytekit/core/base_task.py
- flytekit/core/python_function_task.py
- flytekit/core/task.py
- flytekit/models/task.py
- tests/flytekit/common/parameterizers.py
- tests/flytekit/unit/core/test_python_function_task.py
- tests/flytekit/unit/models/test_tasks.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at haytham@union.ai.

Documentation & Help

AI Code Review powered by

flyte-bot · 2025-03-25T05:57:36Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhanced Task Execution Mode Handling	- `base_task.py` - Introduced 'execution_mode' parameter and default initialization with future annotations import. - `python_function_task.py` - Added conversion methods to map between Python execution behavior and flyteidl execution mode. - `task.py` - Passed execution_mode during task registration to support dynamic workflow behavior. - `task.py` - Integrated execution_mode into TaskMetadata with property, default setting, and conversion logic.
Testing - Updated Test Cases for Execution Mode	- `parameterizers.py` - Modified product iteration to include execution_mode and added related import. - `test_python_function_task.py` - Included tasks_pb2 import to support new execution mode conversion tests. - `test_tasks.py` - Adjusted tuple parameters and added assertions to verify the correct execution_mode behavior.

flyte-bot · 2025-03-25T05:57:37Z

tests/flytekit/common/parameterizers.py

        [True, False],
        ["A", "B"],
        [()],
+        [PythonFunctionTask.ExecutionBehavior.DEFAULT]


Missing comma in product function parameters

The product function is now including execution_mode as a parameter, but the corresponding list in the product call is missing a comma at the end of line 142. This could lead to syntax errors or unexpected behavior when the code is executed.

Code suggestion

Check the AI-generated fix before applying

Suggested change

[PythonFunctionTask.ExecutionBehavior.DEFAULT]

[PythonFunctionTask.ExecutionBehavior.DEFAULT],

Code Review Run #0d112d

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

flyte-bot · 2025-03-25T05:57:38Z

flytekit/core/base_task.py

    pod_template_name: Optional[str] = None
    generates_deck: bool = False
    is_eager: bool = False
+    execution_mode: "PythonFunctionTask.ExecutionBehavior" = None


Undefined name in type annotation

There's an undefined name PythonFunctionTask in the TaskMetadata class. This class is likely imported from another module but the import is missing. Consider adding the appropriate import or using a string literal for the type annotation.

Code suggestion

Check the AI-generated fix before applying

Suggested change

execution_mode: "PythonFunctionTask.ExecutionBehavior" = None

execution_mode: "PythonFunctionTask.ExecutionBehavior" = None

Code Review Run #0d112d

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

flyte-bot · 2025-03-25T05:57:39Z

flytekit/core/python_function_task.py

+            return {
+                tasks_pb2.TaskMetadata.ExecutionMode.DEFAULT: cls.DEFAULT,
+                tasks_pb2.TaskMetadata.ExecutionMode.DYNAMIC: cls.DYNAMIC,
+            }[execution_mode]


Missing fallback for unknown enum values

The from_flyte_idl method doesn't handle potential new execution modes that might be added to the protobuf definition in the future. If a new mode is added to tasks_pb2.TaskMetadata.ExecutionMode but not handled here, it would result in a KeyError.

Code suggestion

Check the AI-generated fix before applying

Suggested change

}[execution_mode]

}.get(execution_mode, cls.DEFAULT) # Default to DEFAULT mode for unknown enum values

Code Review Run #0d112d

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

flyte-bot · 2025-03-25T05:57:40Z

flytekit/models/task.py

+        if self.execution_mode is None:
+            from flytekit.core.python_function_task import PythonFunctionTask
+
+            self.execution_mode = PythonFunctionTask.ExecutionBehavior.DEFAULT


Missing property getter for execution_mode attribute

The __post_init__ method is setting a default value for execution_mode if it's None, but there's no corresponding property getter/setter for execution_mode. This means that self.execution_mode in line 228 will raise an AttributeError since it's trying to access a property that doesn't exist (the actual attribute is _execution_mode). Consider adding a property getter/setter for execution_mode or directly accessing self._execution_mode.

Code suggestion

Check the AI-generated fix before applying

Suggested change

if self.execution_mode is None:

from flytekit.core.python_function_task import PythonFunctionTask

self.execution_mode = PythonFunctionTask.ExecutionBehavior.DEFAULT

if self._execution_mode is None:

from flytekit.core.python_function_task import PythonFunctionTask

self._execution_mode = PythonFunctionTask.ExecutionBehavior.DEFAULT

Code Review Run #0d112d

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

Signed-off-by: Alex Wu <c.alexwu@gmail.com>

codecov · 2025-03-25T08:21:54Z

Codecov Report

Attention: Patch coverage is 45.00000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 35.66%. Comparing base (5503ee5) to head (b206d99).

Files with missing lines	Patch %	Lines
flytekit/models/task.py	33.33%	6 Missing ⚠️
flytekit/core/base_task.py	40.00%	3 Missing ⚠️
flytekit/core/python_function_task.py	66.66%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5503ee5) and HEAD (b206d99). Click for more details.

HEAD has 40 uploads less than BASE

Flag BASE (5503ee5) HEAD (b206d99)

42 2

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3209       +/-   ##
===========================================
- Coverage   81.71%   35.66%   -46.06%     
===========================================
  Files         335      214      -121     
  Lines       27415    22368     -5047     
  Branches     2920     2922        +2     
===========================================
- Hits        22403     7978    -14425     
- Misses       4177    14271    +10094     
+ Partials      835      119      -716

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

flyte-bot · 2025-03-25T14:24:07Z

Code Review Agent Run #032d6a

Actionable Suggestions - 0

Review Details

Files reviewed - 3 · Commit Range: 0bce2cf..b206d99
- flytekit/core/base_task.py
- flytekit/core/python_function_task.py
- tests/flytekit/unit/core/test_python_function_task.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at haytham@union.ai.

Documentation & Help

AI Code Review powered by

popojk added 3 commits March 20, 2025 13:26

fix task template proto

58ddff9

Signed-off-by: Alex Wu <c.alexwu@gmail.com>

resolve conflict

fed6695

Signed-off-by: Alex Wu <c.alexwu@gmail.com>

Merge branch 'master' into Add-support-cache-for-dynamic-spec

0bce2cf

Signed-off-by: Alex Wu <c.alexwu@gmail.com>

popojk requested review from Future-Outlier, cosmicBboy, eapolinario, kumare3, pingsutw, samhita-alla, thomasjpfan and wild-endeavor as code owners March 25, 2025 05:47

popojk mentioned this pull request Mar 25, 2025

Add support cache for dynamic spec flyteorg/flyte#6372

Open

2 tasks

flyte-bot reviewed Mar 25, 2025

View reviewed changes

fix lint error

b206d99

Signed-off-by: Alex Wu <c.alexwu@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support cache for dynamic spec#3209

Add support cache for dynamic spec#3209
popojk wants to merge 4 commits intoflyteorg:masterfrom
popojk:Add-support-cache-for-dynamic-spec

popojk commented Mar 25, 2025 •

edited by flyte-bot

Loading

Uh oh!

welcome bot commented Mar 25, 2025

Uh oh!

flyte-bot commented Mar 25, 2025 •

edited

Loading

Code Review Agent Run #0d112d

Bito Usage Guide

Uh oh!

flyte-bot commented Mar 25, 2025 •

edited

Loading

Changelist by Bito

Uh oh!

flyte-bot Mar 25, 2025

Uh oh!

flyte-bot Mar 25, 2025

Uh oh!

flyte-bot Mar 25, 2025

Uh oh!

flyte-bot Mar 25, 2025

Uh oh!

codecov bot commented Mar 25, 2025 •

edited

Loading

Uh oh!

flyte-bot commented Mar 25, 2025 •

edited

Loading

Code Review Agent Run #032d6a

Bito Usage Guide

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	[PythonFunctionTask.ExecutionBehavior.DEFAULT]
	[PythonFunctionTask.ExecutionBehavior.DEFAULT],

	execution_mode: "PythonFunctionTask.ExecutionBehavior" = None
	execution_mode: "PythonFunctionTask.ExecutionBehavior" = None

	}[execution_mode]
	}.get(execution_mode, cls.DEFAULT) # Default to DEFAULT mode for unknown enum values

Conversation

popojk commented Mar 25, 2025 • edited by flyte-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

Check all the applicable boxes

Related PRs

Summary by Bito

Uh oh!

welcome bot commented Mar 25, 2025

Uh oh!

flyte-bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #0d112d

Bito Usage Guide

Uh oh!

flyte-bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelist by Bito

Uh oh!

flyte-bot Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

flyte-bot Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

flyte-bot Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

flyte-bot Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

flyte-bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #032d6a

Bito Usage Guide

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

popojk commented Mar 25, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Mar 25, 2025 •

edited

Loading

flyte-bot commented Mar 25, 2025 •

edited

Loading

codecov bot commented Mar 25, 2025 •

edited

Loading

flyte-bot commented Mar 25, 2025 •

edited

Loading