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

[Mounts on images] add attach_local_file/attach_local_dir #2507

Draft
wants to merge 4 commits into
base: freider/mount-on-image
Choose a base branch
from

Conversation

freider
Copy link
Contributor

@freider freider commented Nov 14, 2024

Since we create new methods we have the opportunity to introduce otherwise breaking logic changes for where to put/how to names files that have been somewhat inconsistent or poorly documented so far:

Here is the state of various edge case handling of directories and files copying:

Directories

shell

If you have a file source_dir/foo

cp -r source_dir /dest in shell creates a /dest/source_dir/foo

whereas adding a trailing slash on the source:

cp -r source_dir/ /dest in shell creates a /dest/foo

docker

In docker COPY source_dir /dest you always copy the content of the dir and never the dir itself (as if you always had a trailing slash in shell cp source dir).
If you want an output directory with the same name you specify that in the destination path explicitly.

current Modal Mount.from_local_dir

The current way this works in Modal mounts today and in attach_local_dir in this PR is how it works in Docker (always copy contents, never the directory itself).

current Modal image.copy_local_dir

Same as modal.mounts/docker - always copies content into the destination dir

Individual files

shell

cp some_file dest # this creates dest as a file
cp some_file dest/ # this creates dest/some_file and fails if dest doesn't exist

docker

COPY some_file dest # this creates dest as a file
COPY some_file dest/ # this creates dest/some_file and creates the dest dir if it doesn't exist

current Modal Mount.from_local_file

Mount.from_local_file("some_file") # this creates a file named "some_file"
Mount.from_local_file("some_file", remote_path="dest") # this creates a file named /dest
Mount.from_local_file("some_file", remote_path="dest/") # this creates a file named /dest 🤡 🤯

current Modal image.copy_local_file

Image.copy_local_file("some_file") # this creates a file named some_file in workdir
Image.copy_local_file("some_file", remote_path="dest") # this creates a file named dest in workdir
Image.copy_local_file("some_file", remote_path="dest/") # this creates the dest/ dir in workdir and creates a file dest/some_file 👍

Current differences compared to existing copy_ commands:

  • Trailing slash on the remote_path of attach_local_file works the same regardless if we're using mounts or images
  • The current copy_local_ commands support relative destination paths, but attach doesn't (this is fixable, but a bit messy, so I'm leaving it out for now and adding an exception so we can fix it later and people don't rely on the "broken" implementation)

Changelog

  • Adds Image.attach_local_file(..., copy=False) and Image.attach_local_dir(..., copy=False) as a unified replacement for the old Image.copy_local_*() and Mount.add_local_* methods.

@freider freider force-pushed the freider/mount-on-image-add-file branch from 3d2fc82 to 18ca935 Compare November 15, 2024 09:53
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.

1 participant