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.

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

Support FlyteRemote.execute interruptible flag override #2885

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

Conversation

redartera
Copy link
Contributor

@redartera redartera commented Oct 31, 2024

Tracking issue

Closes flyteorg/flyte#5279

Why are the changes needed?

It is useful for users to invoke the same LaunchPlan but overriding at runtime whether they need executions to be interruptible or not. Currently this is possible through the Flyte UI, but not flytekit.

What changes were proposed in this pull request?

Support an interruptible option in FlyteRemote.execute

How was this patch tested?

Integration test added

Check all the applicable boxes

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

Summary by Bito

This PR implements interruptible flag override functionality in FlyteRemote and introduces major enhancements including Environment class, Optuna plugin, and VLLM integration. The changes include runtime control of workflow interruption, improved file handling with pydantic support, enhanced directory handling, and remote execution capabilities. The implementation includes support for nested workflows and adds launch plan registration and file transfer utilities. Core components received bug fixes for pydantic compatibility and logger usage.

Unit tests added: True

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

Signed-off-by: redartera <reda@artera.ai>
Signed-off-by: redartera <reda@artera.ai>
Signed-off-by: redartera <reda@artera.ai>
Signed-off-by: redartera <reda@artera.ai>
Signed-off-by: redartera <reda@artera.ai>
Signed-off-by: redartera <reda@artera.ai>
@redartera redartera marked this pull request as ready for review October 31, 2024 15:11
pingsutw
pingsutw previously approved these changes Nov 4, 2024
flytekit/remote/remote.py Show resolved Hide resolved
@redartera
Copy link
Contributor Author

@pingsutw Can we please merge this PR?

@Future-Outlier Future-Outlier enabled auto-merge (squash) November 21, 2024 13:37
auto-merge was automatically disabled November 21, 2024 13:53

Head branch was pushed to by a user without write access

Signed-off-by: redartera <reda@artera.ai>
@redartera redartera force-pushed the flyteremote-interruptible-override branch from aa09a04 to e82b2ce Compare November 21, 2024 13:55
Signed-off-by: redartera <reda@artera.ai>
@redartera
Copy link
Contributor Author

redartera commented Nov 21, 2024

After merging main into the current branch - the Monodoc build started failing. I added a newline here thinking that was where the problem was coming from which dismissed the approval.

Actually I synched the master branch of my flytekit fork which is identical to the original, and Monodocs fails there too, which makes me think the failure is unrelated to this PR.

@Future-Outlier
Copy link
Member

After merging main into the current branch - the Monodoc build started failing. I added a newline here thinking that was there the problem was coming from which dismissed the approval.

Actually I synched the master branch of my flytekit fork which is identical to the original, and Monodocs fails there too, which makes me think the failure is unrelated to this PR.

yes this is my fault

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>
@redartera redartera requested a review from pingsutw November 25, 2024 15:13
@redartera
Copy link
Contributor Author

@pingsutw @Future-Outlier can you please take another look? the monodocs GH action issue seems resolved now

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.77%. Comparing base (dfa8f04) to head (51c5305).

Files with missing lines Patch % Lines
flytekit/models/execution.py 80.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (dfa8f04) and HEAD (51c5305). Click for more details.

HEAD has 49 uploads less than BASE
Flag BASE (dfa8f04) HEAD (51c5305)
53 4
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2885      +/-   ##
==========================================
- Coverage   83.47%   75.77%   -7.71%     
==========================================
  Files         319      202     -117     
  Lines       26427    21360    -5067     
  Branches     2744     2744              
==========================================
- Hits        22060    16185    -5875     
- Misses       3591     4366     +775     
- Partials      776      809      +33     

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

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 2, 2025

Code Review Agent Run #d114f3

Actionable Suggestions - 1
  • flytekit/remote/remote.py - 1
Additional Suggestions - 1
  • flytekit/models/execution.py - 1
    • Consider adding type hint for interruptible · Line 365-365
Review Details
  • Files reviewed - 3 · Commit Range: 1e775e8..bc025a7
    • flytekit/models/execution.py
    • flytekit/remote/remote.py
    • tests/flytekit/integration/remote/test_remote.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 2, 2025

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Add Interruptible Flag Override Support

execution.py - Added interruptible flag property and handling in ExecutionSpec model

remote.py - Implemented interruptible flag parameter across execute methods

test_remote.py - Added integration tests for interruptible flag override functionality

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 13, 2025

Code Review Agent Run #fb24f7

Actionable Suggestions - 0
Additional Suggestions - 10
  • plugins/flytekit-optuna/flytekitplugins/optuna/optimizer.py - 1
    • Consider requiring positive concurrency value · Line 60-61
  • plugins/flytekit-inference/flytekitplugins/inference/vllm/serve.py - 1
    • Consider moving validation to HFSecret class · Line 44-47
  • flytekit/clis/sdk_in_container/run.py - 1
    • Consider consolidating JSON serialization logic · Line 478-486
  • tests/flytekit/unit/core/test_flyte_file.py - 1
  • flytekit/image_spec/default_builder.py - 1
  • tests/flytekit/integration/remote/test_remote.py - 3
    • Consider adding registration verification assertions · Line 819-819
    • Consider configurable workflow timeout value · Line 122-122
    • Consider extracting duplicate cleanup code · Line 854-887
  • plugins/flytekit-inference/tests/test_vllm.py - 1
    • Consider breaking down long assertion statement · Line 41-41
  • tests/flytekit/integration/remote/workflows/basic/flytefile.py - 1
    • Consider extracting duplicated file reading logic · Line 18-20
Review Details
  • Files reviewed - 30 · Commit Range: bc025a7..51c5305
    • flytekit/__init__.py
    • flytekit/clis/sdk_in_container/run.py
    • flytekit/core/array_node_map_task.py
    • flytekit/core/environment.py
    • flytekit/core/workflow.py
    • flytekit/image_spec/default_builder.py
    • flytekit/remote/remote.py
    • flytekit/types/directory/types.py
    • flytekit/types/file/file.py
    • plugins/flytekit-inference/flytekitplugins/inference/__init__.py
    • plugins/flytekit-inference/flytekitplugins/inference/vllm/serve.py
    • plugins/flytekit-inference/setup.py
    • plugins/flytekit-inference/tests/test_vllm.py
    • plugins/flytekit-onnx-pytorch/dev-requirements.txt
    • plugins/flytekit-optuna/flytekitplugins/optuna/__init__.py
    • plugins/flytekit-optuna/flytekitplugins/optuna/optimizer.py
    • plugins/flytekit-optuna/setup.py
    • plugins/flytekit-optuna/tests/test_optimizer.py
    • plugins/flytekit-spark/tests/test_environment.py
    • pyproject.toml
    • tests/flytekit/integration/remote/test_remote.py
    • tests/flytekit/integration/remote/utils.py
    • tests/flytekit/integration/remote/workflows/basic/attr_access_sd.py
    • tests/flytekit/integration/remote/workflows/basic/flytefile.py
    • tests/flytekit/integration/remote/workflows/basic/pydantic_wf.py
    • tests/flytekit/unit/core/image_spec/test_default_builder.py
    • tests/flytekit/unit/core/test_environment.py
    • tests/flytekit/unit/core/test_flyte_directory.py
    • tests/flytekit/unit/core/test_flyte_file.py
    • tests/flytekit/unit/core/test_workflows.py
  • Files skipped - 3
    • .github/workflows/pythonbuild.yml - Reason: Filter setting
    • plugins/flytekit-inference/README.md - Reason: Filter setting
    • plugins/flytekit-optuna/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

per-execution interruptible flag not piped through via flytekit
4 participants