[feature/flytekit] Smarter copy handling with image spec #3070

taieeuu · 2025-01-18T18:09:37Z

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

If you use the source_root parameter in imagespec, the command execution will use pyflyte-fast-execute; otherwise, it will use pyflyte-execute.

How was this patch tested?

the unit test pass.

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Summary by Bito

Implementation of enhanced container image handling in flytekit with improved validation logic and execution command determination. The system includes Kubernetes StatefulSet Data Service features, custom Python executable support, and smarter ImageSpec configurations. Changes focus on proper handling of container images, None value checks, and logic refinement for pyflyte-execute selection based on source_root parameters. The implementation includes improved error handling and CLI monitoring capabilities with configurable chunk sizes for data persistence.

Unit tests added: True

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

Signed-off-by: taieeuu <[email protected]>

flyte-bot · 2025-01-18T18:09:51Z

Code Review Agent Run #36f32b

Actionable Suggestions - 0

Additional Suggestions - 1

flytekit/tools/translator.py - 1
- Consider simplifying conditional check · Line 173-177

Review Details

Files reviewed - 1 · Commit Range: ef7ca54..ef7ca54
- flytekit/tools/translator.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

AI Code Review powered by

flyte-bot · 2025-01-18T18:12:00Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhanced Container Image Handling	- `translator.py` - Added conditional logic for applying fast execute prefix based on container image specifications

flyte-bot · 2025-01-25T10:57:24Z

Code Review Agent Run #b288dc

Actionable Suggestions - 0

Additional Suggestions - 10

flytekit/core/data_persistence.py - 1
- Consider adding error handling for kwargs · Line 382-383
flytekit/core/array_node_map_task.py - 1
- Consider using constructor for NodeMetadata name · Line 131-132
plugins/flytekit-envd/flytekitplugins/envd/image_builder.py - 1
- Consider more descriptive error message · Line 101-102
plugins/flytekit-envd/tests/test_image_spec.py - 1
- Consider adding assertions for ImageSpec state · Line 136-148
plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/task.py - 1
- Consider using snake_case for attributes · Line 18-26
plugins/flytekit-k8sdataservice/tests/k8sdataservice/test_sensor.py - 1
- Consider restructuring async timeout handling · Line 66-70
plugins/flytekit-omegaconf/flytekitplugins/omegaconf/dictconfig_transformer.py - 1
- Consider consolidating NoneType handling paths · Line 146-147
flytekit/image_spec/image_spec.py - 1
- Consider adding python_exec path validation · Line 93-93
plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/agent.py - 1
- Consider removing unused config variable reference · Line 33-33
flytekit/clis/sdk_in_container/serve.py - 1
- Consider adding port number validation · Line 83-83

Review Details

Files reviewed - 54 · Commit Range: ef7ca54..272b101
- .pre-commit-config.yaml
- Dockerfile.agent
- docs/source/plugins/k8sstatefuldataservice.rst
- flytekit/clis/sdk_in_container/run.py
- flytekit/clis/sdk_in_container/serve.py
- flytekit/core/array_node_map_task.py
- flytekit/core/data_persistence.py
- flytekit/core/node.py
- flytekit/core/python_function_task.py
- flytekit/core/type_engine.py
- flytekit/exceptions/user.py
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
- flytekit/models/security.py
- flytekit/remote/remote_fs.py
- flytekit/tools/translator.py
- flytekit/types/structured/structured_dataset.py
- plugins/flytekit-envd/flytekitplugins/envd/image_builder.py
- plugins/flytekit-envd/tests/test_image_spec.py
- plugins/flytekit-k8sdataservice/dev-requirements.txt
- plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/__init__.py
- plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/agent.py
- plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/k8s/kube_config.py
- plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/k8s/manager.py
- plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/sensor.py
- plugins/flytekit-k8sdataservice/flytekitplugins/k8sdataservice/task.py
- plugins/flytekit-k8sdataservice/setup.py
- plugins/flytekit-k8sdataservice/tests/k8sdataservice/k8s/test_kube_config.py
- plugins/flytekit-k8sdataservice/tests/k8sdataservice/k8s/test_manager.py
- plugins/flytekit-k8sdataservice/tests/k8sdataservice/test_agent.py
- plugins/flytekit-k8sdataservice/tests/k8sdataservice/test_sensor.py
- plugins/flytekit-k8sdataservice/tests/k8sdataservice/test_task.py
- plugins/flytekit-k8sdataservice/tests/k8sdataservice/utils/test_resources.py
- plugins/flytekit-k8sdataservice/utils/infra.py
- plugins/flytekit-k8sdataservice/utils/resources.py
- plugins/flytekit-kf-pytorch/flytekitplugins/kfpytorch/task.py
- plugins/flytekit-kf-pytorch/tests/test_elastic_task.py
- plugins/flytekit-omegaconf/flytekitplugins/omegaconf/dictconfig_transformer.py
- plugins/flytekit-omegaconf/tests/test_dictconfig_transformer.py
- plugins/setup.py
- pydoclint-errors-baseline.txt
- pyproject.toml
- tests/flytekit/clis/sdk_in_container/test_serve.py
- tests/flytekit/integration/remote/test_remote.py
- tests/flytekit/integration/remote/workflows/basic/get_secret.py
- tests/flytekit/unit/core/image_spec/test_default_builder.py
- tests/flytekit/unit/core/test_array_node_map_task.py
- tests/flytekit/unit/core/test_data_persistence.py
- tests/flytekit/unit/core/test_dataclass.py
- tests/flytekit/unit/core/test_generice_idl_type_engine.py
- tests/flytekit/unit/core/test_list.py
- tests/flytekit/unit/core/test_type_engine.py
- tests/flytekit/unit/extras/pydantic_transformer/test_pydantic_basemodel_transformer.py
- tests/flytekit/unit/types/structured_dataset/test_structured_dataset.py
Files skipped - 2
- .github/workflows/pythonbuild.yml - Reason: Filter setting
- plugins/flytekit-k8sdataservice/README.md - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

pingsutw · 2025-01-28T00:32:37Z

flytekit/tools/translator.py

-            container._args = prefix_with_fast_execute(settings, container.args)
+            if (
+                (not hasattr(entity, "container_image"))
+                or (not entity.container_image)


Suggested change

or (not entity.container_image)

or (entity.container_image is not None)

Thank you for your review ~ I fixed it ~

pingsutw · 2025-01-28T00:33:45Z

flytekit/tools/translator.py

+            if (
+                (not hasattr(entity, "container_image"))
+                or (not entity.container_image)
+                or (entity.container_image and not entity.container_image.source_root)


Suggested change

or (entity.container_image and not entity.container_image.source_root)

or (IsInstance(entity.container_image, ImageSpec) and entity.container_image.source_root is not None)

Thank you for your review ~ I fixed it .

Signed-off-by: taieeuu <[email protected]>

flyte-bot · 2025-01-28T03:34:25Z

Code Review Agent Run #70c125

Actionable Suggestions - 1

flytekit/tools/translator.py - 1
- Incorrect ImageSpec source root check · Line 175-176

Review Details

Files reviewed - 1 · Commit Range: 272b101..cf17684
- flytekit/tools/translator.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

AI Code Review powered by

flyte-bot · 2025-01-28T03:36:58Z

flytekit/tools/translator.py

+                or (entity.container_image is None)
+                or (isinstance(entity.container_image, ImageSpec) and entity.container_image.source_root is not None)


Incorrect ImageSpec source root check

The condition for checking ImageSpec source root appears to be inverted. Currently it checks for source_root is not None, but based on the context it should check for source_root is None to properly handle image building cases.

Code suggestion

Check the AI-generated fix before applying

Suggested change

or (entity.container_image is None)

or (isinstance(entity.container_image, ImageSpec) and entity.container_image.source_root is not None)

or (entity.container_image is None)

or (isinstance(entity.container_image, ImageSpec) and entity.container_image.source_root is None)

Code Review Run #70c125

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

wild-endeavor · 2025-01-31T17:38:50Z

let us think about this... we're thinking through what the ideal experience is.

feat: add conditions for executing pyflyte-fast-execute.

ef7ca54

Signed-off-by: taieeuu <[email protected]>

taieeuu requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners January 18, 2025 18:09

taieeuu changed the title ~~[ WIP ][feature/flytekit] Smarter copy handling with image spec~~ [feature/flytekit] Smarter copy handling with image spec Jan 24, 2025

Merge branch 'master' into issue_6094

272b101

pingsutw reviewed Jan 28, 2025

View reviewed changes

taieeuu added 3 commits January 28, 2025 10:31

opt

1e42eb9

Signed-off-by: taieeuu <[email protected]>

no msg

4f31745

Signed-off-by: taieeuu <[email protected]>

opt

cf17684

Signed-off-by: taieeuu <[email protected]>

flyte-bot reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature/flytekit] Smarter copy handling with image spec #3070

[feature/flytekit] Smarter copy handling with image spec #3070

taieeuu commented Jan 18, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 18, 2025 •

edited

Loading

Code Review Agent Run #36f32b

flyte-bot commented Jan 18, 2025 •

edited

Loading

Changelist by Bito

flyte-bot commented Jan 25, 2025 •

edited

Loading

Code Review Agent Run #b288dc

pingsutw Jan 28, 2025

taieeuu Jan 28, 2025

pingsutw Jan 28, 2025

taieeuu Jan 28, 2025

flyte-bot commented Jan 28, 2025 •

edited

Loading

Code Review Agent Run #70c125

flyte-bot Jan 28, 2025

wild-endeavor commented Jan 31, 2025

	or (not entity.container_image)
	or (entity.container_image is not None)

	or (entity.container_image and not entity.container_image.source_root)
	or (IsInstance(entity.container_image, ImageSpec) and entity.container_image.source_root is not None)

		or (entity.container_image is None)
		or (isinstance(entity.container_image, ImageSpec) and entity.container_image.source_root is not None)

[feature/flytekit] Smarter copy handling with image spec #3070

Are you sure you want to change the base?

[feature/flytekit] Smarter copy handling with image spec #3070

Conversation

taieeuu commented Jan 18, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Screenshots

Check all the applicable boxes

Summary by Bito

flyte-bot commented Jan 18, 2025 • edited Loading

Code Review Agent Run #36f32b

flyte-bot commented Jan 18, 2025 • edited Loading

Changelist by Bito

flyte-bot commented Jan 25, 2025 • edited Loading

Code Review Agent Run #b288dc

pingsutw Jan 28, 2025

Choose a reason for hiding this comment

taieeuu Jan 28, 2025

Choose a reason for hiding this comment

pingsutw Jan 28, 2025

Choose a reason for hiding this comment

taieeuu Jan 28, 2025

Choose a reason for hiding this comment

flyte-bot commented Jan 28, 2025 • edited Loading

Code Review Agent Run #70c125

flyte-bot Jan 28, 2025

Choose a reason for hiding this comment

wild-endeavor commented Jan 31, 2025

taieeuu commented Jan 18, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 18, 2025 •

edited

Loading

flyte-bot commented Jan 18, 2025 •

edited

Loading

flyte-bot commented Jan 25, 2025 •

edited

Loading

flyte-bot commented Jan 28, 2025 •

edited

Loading