Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 5 commits into from

Conversation

taieeuu
Copy link
Contributor

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

Tracking issue

Related to issue#6094

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.

image
image

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

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 18, 2025 •
edited
Loading

Code Review Agent Run #36f32b

Actionable Suggestions - 0
Additional Suggestions - 1
  • flytekit/tools/translator.py - 1
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 Bito Logo

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 18, 2025 •
edited
Loading

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

@taieeuu taieeuu changed the title [ WIP ][feature/flytekit] Smarter copy handling with image spec [feature/flytekit] Smarter copy handling with image spec Jan 24, 2025
@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 25, 2025 •
edited
Loading

Code Review Agent Run #b288dc

Actionable Suggestions - 0
Additional Suggestions - 10
  • flytekit/core/data_persistence.py - 1
  • 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
  • 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 Bito Logo

pingsutw
pingsutw reviewed Jan 28, 2025
View reviewed changes
@taieeuu
opt
1e42eb9 
Signed-off-by: taieeuu <[email protected]>
@taieeuu
no msg
4f31745 
Signed-off-by: taieeuu <[email protected]>
@taieeuu
opt
cf17684 
Signed-off-by: taieeuu <[email protected]>
@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 28, 2025 •
edited
Loading

Code Review Agent Run #70c125

Actionable Suggestions - 1
  • flytekit/tools/translator.py - 1
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 Bito Logo

@wild-endeavor
Copy link
Contributor

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

@taieeuu
Copy link
Contributor Author

taieeuu commented Feb 13, 2025 •
edited
Loading

Hi @wild-endeavor
Thanks for the feedback! Do you have any suggestions regarding the current implementation? I'm happy to iterate based on your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pingsutw pingsutw pingsutw left review comments

@flyte-bot flyte-bot flyte-bot left review comments

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

@eapolinario eapolinario Awaiting requested review from eapolinario eapolinario is a code owner

@cosmicBboy cosmicBboy Awaiting requested review from cosmicBboy cosmicBboy is a code owner

@samhita-alla samhita-alla Awaiting requested review from samhita-alla samhita-alla is a code owner

@thomasjpfan thomasjpfan Awaiting requested review from thomasjpfan thomasjpfan is a code owner

@Future-Outlier Future-Outlier Awaiting requested review from Future-Outlier Future-Outlier is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
OSS Contributions
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@taieeuu @flyte-bot @wild-endeavor @pingsutw