Use mount basename handling in copy_local_*

[freider]

A alternative/further mini fix on top of #583

Relies on the logic of the Mount methods to put files/directories into the right basename dir depending on the argument to remote_path (by defaulting to None, like the mount methods).

This makes the default argument (no argument) to copy_local_dir work like the equivalent add_local_dir of Mount, i.e. copies to a directory with the same name as the source, e.g.:

modal.Image.debian_slim().copy_local_dir("config").run_commands("ls -l config")

Instead of copying all the contents of the dir into the current working directory (this can still be performed by using "." as the remote_path explicitly)

[modal-pr-review-automation]

Auto-approved 👍. This diff qualified for automatic approval and doesn't need follow up review.

[erikbern]

Looks good assuming this works against the integration test I just added (but didn't merge) to our internal repo

[freider]

Looks good assuming this works against the integration test I just added (but didn't merge) to our internal repo

It does not pass the last assertion in that test, since this changes how the default remote_path argument works for copy_local_dir():

local_files_stub = Stub(
    image=(
        make_image()
        .copy_local_dir(supports_path, "/xyz")
        .copy_local_file(supports_path / "local_file.txt", "/pqr/banana.txt")
        .copy_local_file(supports_path / "local_file.txt")  # Copy into /root
        .copy_local_dir(supports_path / "dir")  # Copy into /root too
    )
)

def test_copy_local_file(client):
    with local_files_stub.run(client):
        assert read_file.call("/xyz/local_file.txt") == "Hello, world!\n"
        assert read_file.call("/pqr/banana.txt") == "Hello, world!\n"
        assert read_file.call("/local_file.txt") == "Hello, world!\n"

        # the following breaks, since the file ends up in "/dir/another_file.txt"
        assert read_file.call("/another_file.txt") == "Hello, another world!\n"   

The inconsistency with Mount.from_local_dir/add_local_dir is a bit annoying to me (and I have a slight preference towards defaulting to a directory with the same name as the source dir rather than moving all individual files to root), but at this point we're bound to break some peoples code regardless how we change this, unless we change names of the functions 🤔

[erikbern]

I'm fine breaking stuff if it makes it more consistent. It's not very obvious what should happen with some of those cases. Minor preference for trying to keep it consistent with Docker though

[freider]

I'm fine breaking stuff if it makes it more consistent. It's not very obvious what should happen with some of those cases. Minor preference for trying to keep it consistent with Docker though

The COPY command in docker doesn't have a default destination, and this change shouldn't affect explicit destinations, so it should still be docker-consistent.

The intention is that it should reuse a bit more of the existing from_local/add_local mount code and the "breaking" change is that copying a directory without an explicit remote_path argument will now use the directory basename as the destination rather than "spreading out all of the content", which is consistent with how we do it for Mount.from_local_dir and probably what people want in most cases.

Another unfortunate side effect is that it will cause image rebuilds in places where people use copy_local_dir, since the docker commands/paths change slightly (even if the end result is the same).

If we still want this, lets schedule it for a version bump. Perhaps we should release another patch first where we add a warning to people using copy_local_dir without remote_path, warning them about the upcoming change?

[freider]

Just realized there is a bug here if users change workdir during their image building. Adding some additional integration tests to prevent regressions going forward