Mounts on images [CLI-4] - `image.add_local_python_package` #2343

freider · 2024-10-16T12:05:21Z

Adds the notion of a "mount layer" on Image, which is a way for image objects to track mounts that should be attached to any container that uses the image.
To prevent issues with overlapping file system changes in case of image steps that modify files specified in mount layers, a mount layer is always "materialized" into an actual image layer using "COPY" commands in case there is a subsequent non-mount layer added to the image.

For now, the only place we use these mount layers is in the new image.add_local_python_packages() method on image, but the plan is to roll out changes that replace all current user usages of Mount (add_local_dir, add_local_file etc.) with mount layers and eventually get rid of Mounts as an external concept.

Note that since the attached mounts are entirely a client-side concept, a hypothetical Image.from_id or similar would not be able to recover the mount information, but I don't think we use that anywhere where that would make sense.

Backward/forward compatibility checks

---

Check these boxes or delete any item (or this section) if not relevant for this PR.

Client+Server: this change is compatible with old servers
Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.

Changelog

Adds Image.add_local_python_packages which works similarly to Mount.from_local_python_packages but for images.

…n-image

freider · 2024-11-06T15:31:55Z

modal/image.py

+            raise InvalidError(
+                textwrap.dedent(
+                    """
+                It's recommended to run any `image.add_*` commands for adding local resources last


Suggestions for how to make this wall of text easy to understand would be appreciated :D

freider · 2024-11-20T11:38:18Z

modal/image.py

+            raise InvalidError(
+                textwrap.dedent(
+                    """
+            An image tried to run a build step after using `image.add_local_*` to include local files.


@mwaskom any feedback on this error message copy?

Not directly related but one sort of annoying thing is that verbose error messages end up looking bad in the terminal since we print them twice:

Also can we error on the side of shorter lines? (Or longer lines and let rich handle the wrapping?) With narrow terminals this gets a little garbled:

freider · 2024-11-20T11:54:36Z

modal/functions.py

@@ -633,6 +634,7 @@ def from_args(
        if proxy:
            # HACK: remove this once we stop using ssh tunnels for this.
            if image:
+                # TODO(elias): this will cause an error if users use prior `.add_local_*` commands without copy=True


I think we can remove the autossh installation - checking with @pawalt and @luiscape now

mwaskom

I think we can probably do a little more here to help users understand this. Some of that would be aligning on clear terminology and using it consistently. It would also be good to add a conceptual explanation to the "Guide" page about containers (although it's getting so long now 😠; we need to refactor it) that we can point to.

Also the more I think about it, I wonder if it's misleading to talk about this in terms of "Python packages" when it really is just "add an entire directory tree where a Python module resides". Although that would run counter to my desire for a shorter method name 😁. But maybe this is a good opportunity to think a little bit more about the semantics of this helper function?

mwaskom · 2024-11-20T13:29:00Z

modal/image.py

+            self2._mounts = tuple(base_image._mounts) + (mount,)
+            self2._used_local_mounts = base_image._used_local_mounts | ({mount} if mount.is_local() else set())
+
+        return _Image._from_loader(_load, "ImageWithMounts()", deps=lambda: [base_image, mount])


Is ImageWithMounts() a new class?

No, they just use the regular Image class - this is the repr string. Might have changed the repr it here for debugging reasons (would be nice to fix our object reprs to actually be meaningful/using some data from the specification)

(would be nice to fix our object reprs to actually be meaningful/using some data from the specification)
+1 yeah have been wanting to do this for a while, they could be a lot more userful

mwaskom · 2024-11-20T13:31:04Z

modal/image.py

@@ -553,6 +624,23 @@ def build_dockerfile(version: ImageBuilderVersion) -> DockerfileSpec:
            context_mount=mount,
        )

+    def add_local_python_packages(self, *packages: Union[str, Path], copy: bool = False) -> "_Image":


Since this is a Python API, would add_local_packages save users some typing?

Yeah, but what if the user has non-python packages locally (.deb?) that they want to add? 😬

I agree it's a bit on the long end, but this also reflects the existing Mount.from_local_python_packages

Did some more thinking... see below

mwaskom · 2024-11-20T13:31:42Z

modal/image.py

@@ -553,6 +624,23 @@ def build_dockerfile(version: ImageBuilderVersion) -> DockerfileSpec:
            context_mount=mount,
        )

+    def add_local_python_packages(self, *packages: Union[str, Path], copy: bool = False) -> "_Image":
+        """Attaches local Python packages to the container running the image


Should we avoid introducing new terminology ("attach") if we're not otherwise going to use it?

Something that surprised me when testing — but maybe it shouldn't have — is that adding a local package (really a third-party package; I was just reaching for something quick) didn't include its dependencies.

That makes sense from one perspective (this operation is more like copying a directory with some sugar to reference the directory via PYTHONPATH) but I wonder if it will surprise users? Since "adding a package" usually has some notion of dependency resolution.

"Attach" was an oversight when I renamed it again, thanks for spotting!

Yeah, adding third party packages or packages that have real install recipes via this method isn't intended, but I can see how people would do that. The main intent is for it to be used for directories that are part of the user's current python projet, and the interface has been using the module name as a convenience for pointing to that directory via the current sys.path.

Basically a short form for: add_local_dir(lookup_package("my_package"), filter=modal.file_filters.PyFiles) (assuming we had lookup_package() and filters...

I have separately been thinking that it would be nice to just point to a "project directory" when setting up the user's image, which is very similar to this but using file paths, and maybe that's a better way to do this?
The problem with that is that specifying file system paths in a portable way is so un-ergonomic for even simple cases, e.g:

E.g.

Image.debian_slim().add_local_dir( Path(__file__).parent, remote_path="/root/some_root_package_name", condition=python_mount_condition )

Preferably I'd want it to work like above with default arguments of using:

Image.debian_slim().add_project_dir(".") # use directory of current file as project dir - "standard", only include python files (or .modalignore?)

And then have some very easy ways to customize this:

Image.debian_slim().add_project_dir("..") # use parent directory as root, using relative paths from the current file's dir or cwd? (the latter probably cleanest to implement)

Image.debian_slim().add_project_dir(".", filter=modal.filters.AllFiles) # use all files instead of just python files # this is where we could also take advantage of new glob/modalignore support

I'll rename add_local_python_packages to a "private _add_local_python_packages for now to get the rest of the changes merged in while we discuss the best way of "adding my local python files"

mwaskom · 2024-11-20T13:32:24Z

modal/image.py

+        By default (copy=False) the packages are layered on top of the image when
+        the container starts up and not built as an image layer.


I'm having trouble parsing "packages are layered ... not built as an image layer".

Rewrote:

Adds Python Package Files to Containers

Adds all files from the specified Python packages to containers running the image.

Packages are added to the /root directory, which is on the PYTHONPATH of any executed Modal functions.

By default (copy=False), the files are added to containers on startup and are not built into the actual image, which speeds up deployment.

Set copy=True to copy the files into an image layer at build time instead. This can slow down iteration since it requires a rebuild of the image and any subsequent build steps whenever the included files change, but it is required if you want to run additional build steps after this one.

Note: This excludes all dot-prefixed subdirectories or files and all .pyc/__pycache__ files. To add full directories with finer control, use .add_local_dir() instead.

mwaskom · 2024-11-20T13:43:17Z

test/image_test.py

+
+
+def hydrate_image(img, client):
+    # there should be a more straight forward way to do this?


mwaskom · 2024-11-20T13:44:05Z

modal/image.py

        self._used_local_mounts = frozenset()
+        self._mounts = ()


What's the difference between _used_local_mounts and _mounts?

Ah, there is a docstring further up, but I renamed them now to make it clearer:

_used_local_mounts -> _serve_mounts # used for modal serve mount watching - also includes any context mounts for non-add operation, like copy_...
_mounts -> _deferred_mounts # these are the new "lazy" add to container on startup-mounts introduced by this pr

mwaskom · 2024-11-20T13:50:48Z

modal/image.py

+        Packages are added to the /root directory which is on the PYTHONPATH of any
+        executed Modal functions.


Clarify that this is in the container?

Also might want to use capitalization for our proper nouns to Jono stays happy :)

Which nouns? Function?

mwaskom · 2024-11-20T13:59:00Z

modal/image.py

+            raise InvalidError(
+                textwrap.dedent(
+                    """
+            An image tried to run a build step after using `image.add_local_*` to include local files.


Not directly related but one sort of annoying thing is that verbose error messages end up looking bad in the terminal since we print them twice:

Also can we error on the side of shorter lines? (Or longer lines and let rich handle the wrapping?) With narrow terminals this gets a little garbled:

mwaskom · 2024-11-20T13:59:56Z

modal/image.py

+            filechange. Modal will then mount these files as a thin layer when starting your container, saving build
+            time.


Is the "thin layer" terminology used anywhere else?

Nope, changed

mwaskom · 2024-11-20T22:09:49Z

modal/image.py

+        required package files change, but it allows you to run additional build
+        steps after this operation.
+        """
+        mount = _Mount.from_local_python_packages(*packages)


Should we take the opportunity to break with the existing behavior of including data assets and everything else within the directory containing the Python package, at least by default? That's one of our most surprising and problem-causing behaviors IMO.

I think this is the current behavior for explicit from_local_packages and by this code. The only things that are excluded are .-prefixed files/directories and __pycache__/.pyc.

It's the auto mounts today that exclude everything that's not a .py-file. Very consistent, I know...

It's the auto mounts today that exclude everything that's not a .py-file.

I thought auto mounting now included most files? That's how users get themselves into trouble when they automount a project directory that has a lot of data in it.

I agree that you're mostly mirroring the existing behavior of Mount.from_local_python_packages but I'm wondering should we take this opportunity to break things about that behavior that have proven problematic.

(I also think we should add the dockerignore conditions to these interfaces, which will help a bit, but my suspicion is that it's easier to debug "this add_local_python_packages command is not including my data files" than "my Image build is really slow and seems to be uploading a lot of stuff". But I'm not totally sure.)

freider added 29 commits September 2, 2024 15:58

wip

8bc041c

wip

b8d399f

wip

5e34e80

wip

af9f74f

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

0b40907

Working consolidation of mounts

2a52f6d

Functions now attach lazy mounts to the container

e237634

Fix sandboxes

787dcc6

Fix env client issue

c89b7a4

Disable test on windows

244af57

types

813d3f1

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

77186d0

Disable auto mounts in image tests, trims ~30s of test time

36bc55e

Make materialization an explicit step

14907e5

comment

19780c7

wip refactor

18d931f

Fixes

01aab3b

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

cab50ff

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

25b388b

Fixing tests

0abd75b

Adds more watcher tests, including failing one

6e2db24

wip

2ffac62

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

7e41f47

Move mount extraction logic post-app-load, include image mounts

6b107a1

if

dfc627b

Merge branch 'freider/image-mount-watch-cli-219' into freider/mount-o…

fef3f9c

…n-image

Add test + fix issue with classes

908b329

Merge branch 'freider/image-mount-watch-cli-219' into freider/mount-o…

7c81d3f

…n-image

Fix tests

6b954c9

freider changed the base branch from main to freider/image-mount-watch-cli-219 November 6, 2024 15:16

freider commented Nov 6, 2024

View reviewed changes

freider added 2 commits November 6, 2024 16:35

Cleanup

8f81737

Copy

7ccb07a

freider requested a review from mwaskom November 6, 2024 15:48

Base automatically changed from freider/image-mount-watch-cli-219 to main November 12, 2024 13:32

freider added 3 commits November 15, 2024 10:33

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

3a8409c

Rename to attach

fd39606

Copy

d3a4dec

freider changed the title ~~Mounts on images [CLI-4] - image.add_local_python_package~~ Mounts on images [CLI-4] - image.attach_local_python_package Nov 15, 2024

freider added 4 commits November 15, 2024 10:44

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

4a4257d

More name changes

1e1bc97

Back to add

286db82

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

543ee3a

freider changed the title ~~Mounts on images [CLI-4] - image.attach_local_python_package~~ Mounts on images [CLI-4] - image.add_local_python_package Nov 19, 2024

more

652fd7d

freider mentioned this pull request Nov 20, 2024

Adds Image.copy_local_python_packages #638

Closed

freider commented Nov 20, 2024

View reviewed changes

mwaskom approved these changes Nov 20, 2024

View reviewed changes

mwaskom reviewed Nov 20, 2024

View reviewed changes

freider added 8 commits November 21, 2024 11:28

Adjust error message to fewer explicit line breaks

8645fbb

change repr

222dc25

docstring

67ed484

Docstring

c0d8236

Renames and more copy changes

11452a8

More copy

4878945

More copy

94f9843

Merge remote-tracking branch 'origin/main' into freider/mount-on-image

f83f4a8

freider merged commit 49d2962 into main Nov 21, 2024
21 checks passed

freider deleted the freider/mount-on-image branch November 21, 2024 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mounts on images [CLI-4] - `image.add_local_python_package` #2343

Mounts on images [CLI-4] - `image.add_local_python_package` #2343

freider commented Oct 16, 2024 •

edited

Loading

freider Nov 6, 2024

freider Nov 20, 2024

mwaskom Nov 20, 2024

freider Nov 20, 2024

mwaskom left a comment •

edited

Loading

mwaskom Nov 20, 2024

freider Nov 21, 2024

freider Nov 21, 2024

mwaskom Nov 21, 2024

mwaskom Nov 20, 2024

freider Nov 21, 2024

freider Nov 21, 2024

mwaskom Nov 20, 2024

mwaskom Nov 20, 2024

freider Nov 21, 2024 •

edited

Loading

freider Nov 21, 2024

mwaskom Nov 20, 2024

freider Nov 21, 2024

mwaskom Nov 20, 2024

mwaskom Nov 20, 2024

freider Nov 21, 2024

mwaskom Nov 20, 2024

freider Nov 21, 2024

freider Nov 21, 2024

mwaskom Nov 20, 2024

mwaskom Nov 20, 2024

freider Nov 21, 2024

mwaskom Nov 20, 2024

freider Nov 21, 2024

mwaskom Nov 21, 2024 •

edited

Loading

		By default (copy=False) the packages are layered on top of the image when
		the container starts up and not built as an image layer.



		def hydrate_image(img, client):
		# there should be a more straight forward way to do this?

		Packages are added to the /root directory which is on the PYTHONPATH of any
		executed Modal functions.

		filechange. Modal will then mount these files as a thin layer when starting your container, saving build
		time.

Mounts on images [CLI-4] - image.add_local_python_package #2343

Mounts on images [CLI-4] - image.add_local_python_package #2343

Conversation

freider commented Oct 16, 2024 • edited Loading

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaskom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freider Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaskom Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Mounts on images [CLI-4] - `image.add_local_python_package` #2343

Mounts on images [CLI-4] - `image.add_local_python_package` #2343

freider commented Oct 16, 2024 •

edited

Loading

mwaskom left a comment •

edited

Loading

freider Nov 21, 2024 •

edited

Loading

mwaskom Nov 21, 2024 •

edited

Loading