-
Notifications
You must be signed in to change notification settings - Fork 40
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
@prbot approve |
There was a problem hiding this 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.
mwaskom
approved these changes
Nov 22, 2024
There was a problem hiding this 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!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 shellcp
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 createsdest
as a filecp some_file dest/
# this createsdest/some_file
and fails if dest doesn't existdocker
COPY some_file dest
# this createsdest
as a fileCOPY some_file dest/
# this createsdest/some_file
and creates thedest
dir if it doesn't existcurrent 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 /destMount.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 workdirImage.copy_local_file("some_file", remote_path="dest")
# this creates a file named dest in workdirImage.copy_local_file("some_file", remote_path="dest/")
# this creates thedest/
dir in workdir and creates a file dest/some_file 👍Current differences compared to existing copy_ commands:
add_local_file
works the same regardless if we're using mounts or imagescopy_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
Image.add_local_file(..., copy=False)
andImage.add_local_dir(..., copy=False)
as a unified replacement for the oldImage.copy_local_*()
andMount.add_local_*
methods.