Add pip_extra_args #3081

amitani · 2025-01-23T19:23:10Z

Tracking issue

Why are the changes needed?

We have a few path dependencies in pyproject.toml and poetry.lock, and only want to install packages from external sources, which requires adding --no-directory to poetry install. This is a standard practice when installing dependencies without code.
See
https://python-poetry.org/docs/faq/#poetry-busts-my-docker-cache-because-it-requires-me-to-copy-my-source-files-in-before-installing-3rd-party-dependencies

What changes were proposed in this pull request?

pip_extra_args is a specifiable parameter for ImageSpec, and appended when running poetry install, uv sync and pip install.

How was this patch tested?

I tested locally with a poetry project with internal path dependency by running pyflyte build. Planning to add unit tests once the implementation get LGTM.

Setup process

Screenshots

Check all the applicable boxes

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

Related PRs

#3025

Docs link

Summary by Bito

This pull request introduces a new feature by adding the pip_extra_args parameter to the ImageSpec class, enhancing package installation flexibility. Modifications are made to the image_spec module, specifically in default_builder.py and image_spec.py, to support this new parameter.

Unit tests added: False

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

welcome · 2025-01-23T19:23:13Z

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-01-23T19:23:26Z

Code Review Agent Run #bf76cd

Actionable Suggestions - 1

flytekit/image_spec/image_spec.py - 1
- Missing class attribute for docstring · Line 51-51

Review Details

Files reviewed - 2 · Commit Range: 2569f80..2569f80
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.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-23T19:26:07Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
New Feature - Addition of pip_extra_args Parameter	- `default_builder.py` - Added handling for pip_extra_args in prepare_python_install function. - `image_spec.py` - Introduced pip_extra_args as an optional parameter in ImageSpec.

flytekit/image_spec/image_spec.py

thomasjpfan

Can you update a list to make sure that the new argument is used?

thomasjpfan · 2025-01-23T21:18:47Z

flytekit/image_spec/image_spec.py

@@ -82,6 +83,7 @@ class ImageSpec:
    platform: str = "linux/amd64"
    pip_index: Optional[str] = None
    pip_extra_index_url: Optional[List[str]] = None
+    pip_extra_args: Optional[List[str]] = None


Can we make this a single string and not a list?

Suggested change

pip_extra_args: Optional[List[str]] = None

pip_extra_args: Optional[str] = None

I made the update to take a space-delimited string. Just curious, but what's the rationale behind this suggestion? pip_install_args is joined with " " later, so taking a list and extending the list seems more aligned with how it is implemented.

For example, when adding multiple command line arguments, it is done by extending the list of multiple elements, not a single element with multiple command line arguments concatenated with spaces.

flytekit/flytekit/image_spec/default_builder.py

Line 195 in 5a2c44f

pip_install_args.extend(["--locked", "--no-dev", "--no-install-project"])

(a separate topic but it may be worth using shlex instead of " ".join)

Signed-off-by: Akinori Mitani <akinori@artera.ai>

flyte-bot · 2025-01-23T22:35:29Z

Code Review Agent Run #110b04

Actionable Suggestions - 1

flytekit/image_spec/default_builder.py - 1
- Potential issue with pip_extra_args handling · Line 220-220

Review Details

Files reviewed - 2 · Commit Range: 4281029..4281029
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.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-23T22:37:53Z

flytekit/image_spec/default_builder.py

@@ -215,6 +215,9 @@ def prepare_python_install(image_spec: ImageSpec, tmp_dir: Path) -> str:
    if image_spec.pip_extra_index_url:
        extra_urls = [f"--extra-index-url {url}" for url in image_spec.pip_extra_index_url]
        pip_install_args.extend(extra_urls)
+
+    if image_spec.pip_extra_args:
+        pip_install_args.append(image_spec.pip_extra_args)


Potential issue with pip_extra_args handling

Consider checking if image_spec.pip_extra_args is a list or a string. If it's a string, it should be split into individual arguments before appending to pip_install_args.

Code suggestion

Check the AI-generated fix before applying

Suggested change

pip_install_args.append(image_spec.pip_extra_args)

if isinstance(image_spec.pip_extra_args, str):

pip_install_args.extend(image_spec.pip_extra_args.split())

else:

pip_install_args.append(image_spec.pip_extra_args)

Code Review Run #110b04

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

it was incorrectly flagged

amitani requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners January 23, 2025 19:23

flyte-bot reviewed Jan 23, 2025

View reviewed changes

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

thomasjpfan reviewed Jan 23, 2025

View reviewed changes

Add pip_extra_args

4281029

Signed-off-by: Akinori Mitani <akinori@artera.ai>

amitani force-pushed the pip_extra_args branch from 5a2c44f to 4281029 Compare January 23, 2025 21:42

flyte-bot reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pip_extra_args #3081

Add pip_extra_args #3081

amitani commented Jan 23, 2025 •

edited by flyte-bot

Loading

welcome bot commented Jan 23, 2025

flyte-bot commented Jan 23, 2025 •

edited

Loading

Code Review Agent Run #bf76cd

flyte-bot commented Jan 23, 2025

Changelist by Bito

thomasjpfan left a comment

thomasjpfan Jan 23, 2025

amitani Jan 23, 2025 •

edited

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

Code Review Agent Run #110b04

flyte-bot Jan 23, 2025

	pip_extra_args: Optional[List[str]] = None
	pip_extra_args: Optional[str] = None

Add pip_extra_args #3081

Are you sure you want to change the base?

Add pip_extra_args #3081

Conversation

amitani commented Jan 23, 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?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Summary by Bito

welcome bot commented Jan 23, 2025

flyte-bot commented Jan 23, 2025 • edited Loading

Code Review Agent Run #bf76cd

flyte-bot commented Jan 23, 2025

Changelist by Bito

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan Jan 23, 2025

Choose a reason for hiding this comment

amitani Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

flyte-bot commented Jan 23, 2025 • edited Loading

Code Review Agent Run #110b04

flyte-bot Jan 23, 2025

Choose a reason for hiding this comment

amitani commented Jan 23, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

amitani Jan 23, 2025 •

edited

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading