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_local_file/add_local_dir #2507

Merged
merged 61 commits into from
Nov 23, 2024

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 my attempt at an overview of 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 add_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 add_local_file works the same regardless if we're using mounts or images
  • The current copy_local_ commands support relative destination paths, but add doesn't. This is fixable, but a bit messy, so I'm leaving it out for now and adding an exception when using relative paths so we can fix it later and people don't rely on the "broken" implementation.

Changelog

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

Base automatically changed from freider/mount-on-image to main November 21, 2024 13:53
@freider freider marked this pull request as ready for review November 21, 2024 14:42
@freider freider requested a review from mwaskom November 21, 2024 14:42
@mwaskom mwaskom changed the title [Mounts on images] add attach_local_file/attach_local_dir [Mounts on images] add_local_file/add_local_dir Nov 21, 2024
@freider
Copy link
Contributor Author

freider commented Nov 22, 2024

@prbot approve

Copy link

@modal-pr-review-automation modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

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

Approved 👍. @mwaskom will follow-up review this.

Copy link
Contributor

@mwaskom mwaskom left a comment

Choose a reason for hiding this comment

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

Docstrings look clear to me!

@freider freider merged commit a9ea369 into main Nov 23, 2024
22 checks passed
@freider freider deleted the freider/mount-on-image-add-file branch November 23, 2024 19:30
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.

2 participants