Adds ImageSpec.with_runtime_packages by thomasjpfan · Pull Request #3231 · flyteorg/flytekit

thomasjpfan · 2025-04-15T17:59:11Z

Why are the changes needed?

This PR adds the ability to install runtime python dependencies.

from flytekit import ImageSpec, task

image = ImageSpec(base_image="localhost:30000/flytekit:0.1.5", builder="noop")


@task(container_image=image.with_runtime_packages(["arize-phoenix-client"]))
def check_phoenix() -> int:
    from phoenix.client import Client

    return 10

What changes were proposed in this pull request?

This PR adds:

A builder="noop" that returns the original base_image
A imagespec.with_dev_packages(...) to install packages during runtime.
Passes the dev packages through an environment variable.
Updates the entrypoint to install the packages.

How was this patch tested?

I built the Dockerfile.dev and ran the above task.

Summary by Bito

This PR adds support for runtime Python dependencies through a new runtime_packages parameter in ImageSpec. It enhances functionality by modifying the entrypoint script for handling subprocess signals, updating container configuration for dependency variables, and providing a mechanism for dynamically installing required packages at runtime, streamlining dependency management.

Unit tests added: True

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

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

codecov · 2025-04-15T18:37:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.64%. Comparing base (8abecfe) to head (b6ce00d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3231      +/-   ##
==========================================
+ Coverage   89.81%   91.64%   +1.83%     
==========================================
  Files         136      131       -5     
  Lines        6599     5890     -709     
==========================================
- Hits         5927     5398     -529     
+ Misses        672      492     -180

☔ 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-04-15T18:58:47Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - Bito Automatic Review Skipped - Draft PR

Bito didn't auto-review because this pull request is in draft status.
To trigger review, mark the PR as ready or type /review in the comment and save.
You can change draft PR review settings here, or contact the agent instance creator at eduardo@union.ai.

flyte-bot · 2025-04-15T19:12:16Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - Bito Automatic Review Skipped - Draft PR

Bito didn't auto-review because this pull request is in draft status.
To trigger review, mark the PR as ready or type /review in the comment and save.
You can change draft PR review settings here, or contact the agent instance creator at eduardo@union.ai.

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

flyte-bot · 2025-04-15T20:55:43Z

Code Review Agent Run #b1335b

Actionable Suggestions - 1

flytekit/bin/entrypoint.py - 1
- Signal handler not restored after subprocess completes · Line 68-75

Additional Suggestions - 10

flytekit/image_spec/noop_builder.py - 2
- Import statement placed inside method body · Line 14-16
- Use TypeError for invalid type checks · Line 12-12
flytekit/image_spec/__init__.py - 1
- NoOpBuilder registration with lower priority is redundant · Line 20-20
flytekit/bin/entrypoint.py - 3
- Use list instead of List annotation · Line 66-66
- Missing return type for private function · Line 70-70
- Missing return type for array index function · Line 78-78
flytekit/core/python_auto_container.py - 1
- Move imports to type-checking block · Line 12-12
flytekit/image_spec/image_spec.py - 3
- Missing return type and docstring · Line 106-106
- Missing trailing comma in dict factory · Line 174-174
- Use modern Python type annotations · Line 102-104

Review Details

Files reviewed - 9 · Commit Range: b5a1aa9..d022e45
- flytekit/bin/entrypoint.py
- flytekit/core/constants.py
- flytekit/core/python_auto_container.py
- flytekit/image_spec/__init__.py
- flytekit/image_spec/image_spec.py
- flytekit/image_spec/noop_builder.py
- pydoclint-errors-baseline.txt
- tests/flytekit/unit/core/image_spec/test_image_spec.py
- tests/flytekit/unit/core/image_spec/test_noop_builder.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-04-15T21:06:38Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
New Feature - Runtime Dependency Installation Feature	- `entrypoint.py` - Introduced a new helper (_run_subprocess) for proper SIGTERM handling and updated the logic to install runtime packages via an environment variable. - `constants.py` - Added a new constant (RUNTIME_PACKAGES_ENV_NAME) to support runtime package installation. - `python_auto_container.py` - Enhanced the container setup to propagate runtime package information into the environment. - `image_spec.py` - Added a new optional field (runtime_packages) and a with_runtime_packages method to enable runtime dependency management. - `noop_builder.py` - Implemented a NoOpBuilder that uses the base image without further modifications, supporting the runtime packages feature. - `__init__.py` - Registered the NoOpBuilder ensuring it is available for runtime package installation workflows. - `test_image_spec.py` - Included tests verifying that the with_runtime_packages method correctly attaches runtime dependencies. - `test_noop_builder.py` - Added tests to confirm NoOpBuilder enforces base_image type requirements and correctly handles runtime packages.
Documentation - Updates on Documentation for ImageSpec	- `pydoclint-errors-baseline.txt` - Adjusted documentation and linting messages for the ImageSpec class to reflect the inclusion of the new runtime_packages attribute.

flyte-bot · 2025-04-15T21:06:40Z

+    p = subprocess.Popen(cmd, env=env)
+
+    def handle_sigterm(signum, frame):
+        logger.info(f"passing signum {signum} [frame={frame}] to subprocess")
+        p.send_signal(signum)
+
+    signal.signal(signal.SIGTERM, handle_sigterm)
+    return p.wait()


Signal handler not restored after subprocess completes

The _run_subprocess function doesn't restore the original signal handler after the subprocess completes, which could affect signal handling in the parent process. Consider saving and restoring the original handler.

Code suggestion

Check the AI-generated fix before applying

Suggested change

p = subprocess.Popen(cmd, env=env)

def handle_sigterm(signum, frame):

logger.info(f"passing signum {signum} [frame={frame}] to subprocess")

p.send_signal(signum)

signal.signal(signal.SIGTERM, handle_sigterm)

return p.wait()

p = subprocess.Popen(cmd, env=env)

original_handler = signal.getsignal(signal.SIGTERM)

def handle_sigterm(signum, frame):

logger.info(f"passing signum {signum} [frame={frame}] to subprocess")

p.send_signal(signum)

signal.signal(signal.SIGTERM, handle_sigterm)

exit_code = p.wait()

signal.signal(signal.SIGTERM, original_handler)

return exit_code

Code Review Run #b1335b

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

Yes, avoid them

fiedlerNr9

LGTM & tested myself ✅

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

flyte-bot · 2025-04-16T15:52:18Z

Code Review Agent Run #2ad2e5

Actionable Suggestions - 1

flytekit/bin/entrypoint.py - 1
- Removed --user flag from pip install command · Line 452-452

Review Details

Files reviewed - 2 · Commit Range: d022e45..2ad32c8
- flytekit/bin/entrypoint.py
- flytekit/core/python_auto_container.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

wild-endeavor · 2025-04-17T17:14:14Z

            If the option is set by the user, then that option is of course used.
        copy: List of files/directories to copy to /root. e.g. ["src/file1.txt", "src/file2.txt"]
        python_exec: Python executable to use for install packages
+        runtime_packages: List of packages to be installed during runtime. `runtime_packages` requires `pip` to be installed


Can we add a comment telling people to not use this? Or use it only as a last resort?

wild-endeavor · 2025-04-17T17:16:10Z

+        logger.info(f"passing signum {signum} [frame={frame}] to subprocess")
+        p.send_signal(signum)
+
+    signal.signal(signal.SIGTERM, handle_sigterm)


should we uninstall this after?

wild-endeavor

+1, couple comments that you can ignore @thomasjpfan

* Adds ImageSpec.with_dev_dependencies Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add tests for noop builder Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Use runtime_packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add docs abount how to use runtime packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Less diffs Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix formatting Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Dix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Let pip default to user by itself to be more compatible Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

* Adds ImageSpec.with_runtime_packages (#3231) * Adds ImageSpec.with_dev_dependencies Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add tests for noop builder Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Use runtime_packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add docs abount how to use runtime packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Less diffs Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix formatting Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Dix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Let pip default to user by itself to be more compatible Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Image spec builder options (#3233) * Image spec builder options Provide the ability to specify image `builder` specific options per Image Spec. Signed-off-by: Mike Hotan <mike@union.ai> * Add builder_options validation Signed-off-by: Mike Hotan <mike@union.ai> * updates Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> Signed-off-by: Mike Hotan <mike@union.ai> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

* Adds ImageSpec.with_dev_dependencies Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add tests for noop builder Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Use runtime_packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add docs abount how to use runtime packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Less diffs Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix formatting Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Dix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Let pip default to user by itself to be more compatible Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> Signed-off-by: Atharva <atharvakulkarni172003@gmail.com>

thomasjpfan added 3 commits April 15, 2025 13:56

Adds ImageSpec.with_dev_dependencies

b5a1aa9

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

Fix

4b0521d

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

Add tests for noop builder

3ea79df

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan added 2 commits April 15, 2025 15:44

Use runtime_packages

e9d384d

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

Add docs abount how to use runtime packages

b9a412a

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan changed the title ~~Adds ImageSpec.with_dev_dependencies~~ Adds ImageSpec.with_runtime_packages Apr 15, 2025

thomasjpfan marked this pull request as ready for review April 15, 2025 19:49

thomasjpfan requested review from Future-Outlier, cosmicBboy, davidmirror-ops, eapolinario, kumare3, pingsutw, samhita-alla and wild-endeavor as code owners April 15, 2025 19:49

thomasjpfan added 4 commits April 15, 2025 15:50

Less diffs

9d19964

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

Fix formatting

b8c2fb2

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

Fix docstring

b6ce00d

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

Dix docstring

d022e45

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

flyte-bot reviewed Apr 15, 2025

View reviewed changes

fiedlerNr9 previously approved these changes Apr 15, 2025

View reviewed changes

Let pip default to user by itself to be more compatible

2ad32c8

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan dismissed fiedlerNr9’s stale review via 2ad32c8 April 16, 2025 14:44

wild-endeavor reviewed Apr 17, 2025

View reviewed changes

flyteorg deleted a comment from flyte-bot Apr 17, 2025

wild-endeavor approved these changes Apr 17, 2025

View reviewed changes

fiedlerNr9 merged commit 9b100f0 into flyteorg:master Apr 18, 2025
116 checks passed

wild-endeavor mentioned this pull request Apr 22, 2025

[v1.15 Backport] image spec builder options 3233 & 3231 #3239

Closed

mhotan mentioned this pull request Apr 22, 2025

mhotan/v1.15 bp 3231 3239 #3240

Merged

Conversation

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

Uh oh!

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Summary by Bito

Uh oh!

codecov Bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Code Review Agent Run Status

Uh oh!

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

Uh oh!

Code Review Agent Run Status

Uh oh!

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

Uh oh!

Code Review Agent Run #b1335b

Uh oh!

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

Uh oh!

Changelist by Bito

Uh oh!

flyte-bot Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

fiedlerNr9 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Code Review Agent Run #2ad2e5

Uh oh!

wild-endeavor Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

wild-endeavor Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

wild-endeavor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thomasjpfan commented Apr 15, 2025 •

edited by flyte-bot

Loading

codecov Bot commented Apr 15, 2025 •

edited

Loading

flyte-bot commented Apr 15, 2025 •

edited

Loading

flyte-bot commented Apr 15, 2025 •

edited

Loading

flyte-bot commented Apr 15, 2025 •

edited

Loading

flyte-bot commented Apr 15, 2025 •

edited

Loading

flyte-bot commented Apr 16, 2025 •

edited

Loading