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

Draft: Adds warnings if automounts seem to be required #2667

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

freider
Copy link
Contributor

@freider freider commented Dec 16, 2024

Proof of concept of warning when packages from global scope imports haven't been added to an image using .add_local_python_source.

This could be used in a longer transition period towards deprecation of "auto mounting".

There are a few things that need to be fixed for this to work well:

  • Iron out the details of how what kind of "entrypoint" mounting we want in both script and module notation modal run/deploy:
    • For example, modal deploy my.pak puts my.pak in sys.modules, and it's not 100% clear if we want it to automatically mount everything in my or just the directory structure + init files required to import my.pak.
    • Even something like modal run myscript.py will actually put myscript in sys.modules as a top level package which would get automounted (but currently deduplicated)
    • If you have a package but still use a "script path" within that package as your modal entrypoint, you risk getting two copies of that module: one at the top level /root/script.py and one in your module structure: /root/mypak/script.py and side effects on the module level would be different between the two - potentially causing bugs. These files should preferably be deduplicated in favor of the module variant, but that isn't done today.
    • There is currently a bug with mount deduplication when using relative paths to refer to imports, e.g. modal run ../foo/bar.py
  • Add a way to specify automounting behavior on an @function (or app) level, to let users silence this warning without resorting to global config/env vars
  • Should we allow explicit auto-mount = on values in the future as well, with all the foot guns that entails? (at least during a transition period it could be nice to have that as a fallback for users once we turn on auto-off)

Check these boxes or delete any item (or this section) if not relevant for this PR.

  • Client+Server: this change is compatible with old servers
  • Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.


Changelog

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