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

adds extra_options arg for pip_install* methods, deprecates injection via packages #1965

Merged
merged 3 commits into from
Jul 5, 2024
Merged

adds extra_options arg for pip_install* methods, deprecates injection via packages #1965

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e26f399
deprecates pip install command-line option injection via packages
charlesfrye Jul 4, 2024
6f37eb1
Apply suggestions from code review
charlesfrye Jul 5, 2024
d3bb3ea
clearer documentation of extra_options
charlesfrye Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions modal/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,29 @@ def is_str_list(x):
return ret


def _validate_packages(packages: List[str]) -> bool:
"""Validates that a list of packages does not contain any command-line options."""
return not any(pkg.startswith("-") for pkg in packages)


def _warn_invalid_packages(old_command: str) -> None:
deprecation_warning(
(2024, 7, 3),
"Passing flags to `pip` via the `packages` argument of `pip_install` is deprecated."
+ " Please pass flags via the `extra_options` argument instead."
+ "\nNote that this will require a rebuild of the image."
charlesfrye marked this conversation as resolved.
Show resolved Hide resolved
+ " To avoid rebuilding, use the exact same `pip install` command via `run_commands`, i.e.:"
charlesfrye marked this conversation as resolved.
Show resolved Hide resolved
+ f'\n`image.run_commands("{old_command}")`',
show_source=False,
)


def _make_pip_install_args(
find_links: Optional[str] = None, # Passes -f (--find-links) pip install
index_url: Optional[str] = None, # Passes -i (--index-url) to pip install
extra_index_url: Optional[str] = None, # Passes --extra-index-url to pip install
pre: bool = False, # Passes --pre (allow pre-releases) to pip install
extra_options: str = "", # Additional options to pass to pip install
) -> str:
flags = [
("--find-links", find_links), # TODO(erikbern): allow multiple?
Expand All @@ -172,6 +190,11 @@ def _make_pip_install_args(
if pre:
args += " --pre"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine not to fix the extra space issue here but can you add a TODO so we remember to do it at some point?


if extra_options:
if args:
args += " "
args += f"{extra_options}"

return args


Expand Down Expand Up @@ -517,6 +540,7 @@ def pip_install(
index_url: Optional[str] = None, # Passes -i (--index-url) to pip install
extra_index_url: Optional[str] = None, # Passes --extra-index-url to pip install
pre: bool = False, # Passes --pre (allow pre-releases) to pip install
extra_options: str = "", # Additional options to pass to pip install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an example that uses this to the docstring so the format is clear?

force_build: bool = False, # Ignore cached builds, similar to 'docker build --no-cache'
secrets: Sequence[_Secret] = [],
gpu: GPU_T = None,
Expand All @@ -535,8 +559,10 @@ def pip_install(

def build_dockerfile(version: ImageBuilderVersion) -> DockerfileSpec:
package_args = " ".join(shlex.quote(pkg) for pkg in sorted(pkgs))
extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre)
extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre, extra_options)
commands = ["FROM base", f"RUN python -m pip install {package_args} {extra_args}"]
if not _validate_packages(pkgs):
_warn_invalid_packages(commands[-1].split("RUN ")[-1])
if version > "2023.12": # Back-compat for legacy trailing space with empty extra_args
commands = [cmd.strip() for cmd in commands]
return DockerfileSpec(commands=commands, context_files={})
Expand All @@ -558,6 +584,7 @@ def pip_install_private_repos(
index_url: Optional[str] = None, # Passes -i (--index-url) to pip install
extra_index_url: Optional[str] = None, # Passes --extra-index-url to pip install
pre: bool = False, # Passes --pre (allow pre-releases) to pip install
extra_options: str = "", # Additional options to pass to pip install
gpu: GPU_T = None,
secrets: Sequence[_Secret] = [],
force_build: bool = False, # Ignore cached builds, similar to 'docker build --no-cache'
Expand Down Expand Up @@ -633,7 +660,7 @@ def build_dockerfile(version: ImageBuilderVersion) -> DockerfileSpec:
f"(echo 'GITLAB_TOKEN env var not set by provided modal.Secret(s): {secret_names}' && exit 1)\"",
)

extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre)
extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre, extra_options)
commands.extend(["RUN apt-get update && apt-get install -y git"])
commands.extend([f'RUN python3 -m pip install "{url}" {extra_args}' for url in install_urls])
if version > "2023.12": # Back-compat for legacy trailing space with empty extra_args
Expand All @@ -658,6 +685,7 @@ def pip_install_from_requirements(
index_url: Optional[str] = None, # Passes -i (--index-url) to pip install
extra_index_url: Optional[str] = None, # Passes --extra-index-url to pip install
pre: bool = False, # Passes --pre (allow pre-releases) to pip install
extra_options: str = "", # Additional options to pass to pip install
force_build: bool = False, # Ignore cached builds, similar to 'docker build --no-cache'
secrets: Sequence[_Secret] = [],
gpu: GPU_T = None,
Expand All @@ -670,7 +698,7 @@ def build_dockerfile(version: ImageBuilderVersion) -> DockerfileSpec:

null_find_links_arg = " " if version == "2023.12" else ""
find_links_arg = f" -f {find_links}" if find_links else null_find_links_arg
extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre)
extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre, extra_options)

commands = [
"FROM base",
Expand Down Expand Up @@ -698,6 +726,7 @@ def pip_install_from_pyproject(
index_url: Optional[str] = None, # Passes -i (--index-url) to pip install
extra_index_url: Optional[str] = None, # Passes --extra-index-url to pip install
pre: bool = False, # Passes --pre (allow pre-releases) to pip install
extra_options: str = "", # Additional options to pass to pip install
force_build: bool = False, # Ignore cached builds, similar to 'docker build --no-cache'
secrets: Sequence[_Secret] = [],
gpu: GPU_T = None,
Expand Down Expand Up @@ -734,7 +763,7 @@ def build_dockerfile(version: ImageBuilderVersion) -> DockerfileSpec:
if dep_group_name in optionals:
dependencies.extend(optionals[dep_group_name])

extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre)
extra_args = _make_pip_install_args(find_links, index_url, extra_index_url, pre, extra_options)
package_args = " ".join(shlex.quote(pkg) for pkg in sorted(dependencies))
commands = ["FROM base", f"RUN python -m pip install {package_args} {extra_args}"]
if version > "2023.12": # Back-compat for legacy trailing space
Expand Down
16 changes: 14 additions & 2 deletions test/image_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,27 @@ def test_image_python_packages(builder_version, servicer, client):
Image.debian_slim()
.pip_install("sklearn[xyz]")
.pip_install("numpy", "scipy", extra_index_url="https://xyz", find_links="https://abc?q=123", pre=True)
.pip_install("flash-attn", extra_options="--no-build-isolation --no-cache-dir")
.pip_install("pandas", pre=True)
)
app.function(image=image)(dummy)
with app.run(client=client):
layers = get_image_layers(image.object_id, servicer)
assert any("pip install 'sklearn[xyz]'" in cmd for cmd in layers[1].dockerfile_commands)
assert any("pip install 'sklearn[xyz]'" in cmd for cmd in layers[3].dockerfile_commands)
assert any(
"pip install numpy scipy --find-links 'https://abc?q=123' --extra-index-url https://xyz --pre" in cmd
for cmd in layers[0].dockerfile_commands
for cmd in layers[2].dockerfile_commands
)
assert any(
"pip install flash-attn --no-build-isolation --no-cache-dir" in cmd for cmd in layers[1].dockerfile_commands
)
assert any("pip install pandas" + 2 * " " + "--pre" in cmd for cmd in layers[0].dockerfile_commands)

with pytest.warns(DeprecationError):
app = App(image=Image.debian_slim().pip_install("--no-build-isolation", "flash-attn"))
app.function()(dummy)
with app.run(client=client):
pass


def test_image_kwargs_validation(builder_version, servicer, client):
Expand Down
Loading