Image.add_local_python_source

[freider]

Reshape _add_local_python_packages into add_local_python_dir and to make it "public".

Won't merge until feedback has been collected and docs updated

Changelog

• Adds Image.add_local_python_source which works similarly to the old and soon-to-be-deprecated Mount.from_local_python_packages but for images. One notable difference is that the new add_local_python_source only includes .py-files by default

[mwaskom]

Just being annoying but is it a problem that "module" technically corresponds to a single file?

[freider]

It's definitely a problem... The naming of this function and argument is super hard :(

A single file module could also be considered a package right, if it's the only top level entry of a project. So maybe add_local_python_package(+-s) is good after all? But it's still potentially confusing since it doesn't "install" anything...

[mwaskom]

I still wonder if we should change the default condition here to include only .py files, or maybe only directories containing .py files or something.

[freider]

I'm open to it! We need to add easy to override filters for users though - @op just ran into issues with this and resorted back to copy_mount(Mount...) to get around the limitations :(

[freider]

Just changed this to only use .py-files. @kramstrom is working on docker glob parsing, and as a followup we could add some sdk-level globbing/include/exclude syntax too to make condition creation simpler

[op]

I used something like this:

def allow_globs(*patterns: str):
    def condition(file_path: str) -> bool:
        path = Path(file_path).relative_to(repo_root)
        for pattern in patterns:
            if fnmatch(str(path), pattern):
                return True
        return False

    return condition


modal_mount = Mount.from_local_dir(
    ".",
    condition=allow_globs(
        "Cargo.toml",
        "ffi/**/*.rs",
        "crates/**/*.rs",
        "crates/**/*.proto",
        "crates/**/*.sql",
        "**/Cargo.toml",
        "**/requirements.dev.txt",
    ),
)

[mwaskom]

Don't forget that we now also have a dockerignore-compliant pattern matcher in our library: https://github.com/modal-labs/modal-client/blob/main/modal/_utils/pattern_matcher.py

[freider]

Oh wow I wasn't aware of this! @kramstrom this sounds very relevant to the stuff you're working on!

[mwaskom]

Yep we'd just held off on figuring out exactly what the API should be (i.e. are we using it for exclusion or inclusion or both? do you pass a list of string patterns or a file? etc.)

[freider]

I just pitched to Kasper that maybe the easiest for users would to have it always specify inclusion, but have "set arithmetic" operators (+ and - for simplicity) that lets you combine them in any way you please in order to build complex include/excludes.

E.g.

expr = files("**/*.py", "*.data") - files("bigblob.data")

[mwaskom]

I wonder if this is confusing since usually the root project would be what gets included through the modal source mount right? Or maybe not if we start disabling automount.

[freider]

Depends. If this is a separate python project from the current one, it wouldn't be included by automounts unless it's imported in global scope locally. If it's the project that the modal app's module is included in, it would be automounted if the modal app is is deployed/run via module path syntax, but not necessarily if it's included via file/path.py.

All of these inconsistencies would get somewhat fixed by removing automount except for the function file itself.

[freider]

At the expense of having to be a bit more explicit whenever your modal function references other local files 😬

[freider]

@prbot approve

[modal-pr-review-automation]

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