Adds warnings if automounts seem to be required #2667

freider · 2024-12-16T13:42:35Z

Adds a warning when packages from global scope imports haven't been added to an image using .add_local_python_source if using the not setting an explicit include_source mode, and not using MODAL_AUTOMOUNT=0.

This will be used in a longer transition period towards deprecation of "auto mounting", getting us to a future with the following standard:

Auto-mounting of first-party packages is disabled by default, except for the function's own package
Function's own package is still auto-mounted by default with the change that it will only include .py-files
Automounting behavior can be specified on a function and/or app level using the include_source=False flag
Auto-mounting can be configured to one of two values: True (new default, similar but not exactly the same as today's automount=0) and False (not even adding the function's own source package/module)

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

Warns users if the modal.Image of a Function/Cls doesn't include all the globally imported "local" modules (using .add_local_python_source()), and the user hasn't explicitly set an include_source value of True/False. This is in preparation for an upcoming deprecation of the current "auto mount" logic.

…ge-mount-warnings

…nts (since it is added by another path)

…ge-mount-warnings

mwaskom · 2025-01-14T13:58:35Z

FWIW I think it's fine to keep "automount" as the terminology. As a standalone verb I think it's a little more clear; "add" could mean so many things.

…iguration

freider · 2025-01-22T14:36:58Z

FWIW I think it's fine to keep "automount" as the terminology. As a standalone verb I think it's a little more clear; "add" could mean so many things.

What about include_source? :)

mwaskom · 2025-01-22T14:50:11Z

That could work too, although I think "auto" is helpfully communicating something in the current name. But maybe less helpful in an explicit parameter than in a config setting I guess.

…iguration

…ckage-mount-warnings

…ge-mount-warnings

freider · 2025-01-30T13:04:24Z

modal/functions.py

@@ -435,7 +435,7 @@ def _bind_method(
        return fun

    @staticmethod
-    def from_args(
+    def from_local(


Semi-unrelated, but I've been wanting to do this rename, and since I replaced _from_args usage in Image I changed this while I was at it...

freider · 2025-01-30T13:08:10Z

modal/image.py

+        img.force_build = force_build
+        return img
+
+    def _extend(self, **kwargs) -> "_Image":


I think we used to have this as a public (?) method, but removed/deprecated it. Now I needed a new private version of it to make it more ergonomic to transfer attributes from parent layers to children (_added_python_source_set in this case)

Is there some reason Image._from_args can't automatically transfer the added source set from the provided base Image?

…ge-mount-warnings

freider · 2025-01-31T12:00:16Z

modal/image.py

@@ -843,7 +853,9 @@ def add_local_python_source(
        ```
        """
        mount = _Mount._from_local_python_packages(*modules, ignore=ignore)
-        return self._add_mount_layer_or_copy(mount, copy=copy)
+        img = self._add_mount_layer_or_copy(mount, copy=copy)
+        img._added_python_source_set |= set(modules)


This is the key part of this change - track all add_local_python_source modules on the Image objects so we can inspect it later when the automount logic executes to see which modules are missing. It's not perfect, but people can get rid of false positive warnings by setting include_source to any boolean value (with the expectation that they know what they are doing)

freider · 2025-01-31T12:03:46Z

modal/_functions.py

+                    deprecation_warning(
+                        (2024, 1, 31),
+                        (
+                            "Automatic adding of local python source will be deprecated in the future.\n"


@mwaskom let me know if you have ideas for other ways of phrasing this, and if we should mention the include_source option or not (I'm leaning towards no since it's not typically needed)

Yeah I think that include_source is not relevant at this stage of the deprecation process; advising users on how to update their Image definitions

Suggested rephrasing of the prose:

Modal will stop implicitly adding local Python modules to the Image ("automounting") in a future update. The following modules need to be explicitly added for future compatibility:

mwaskom · 2025-01-31T15:11:59Z

modal/_functions.py

+                            f"Make sure you have explicitly added the source for the following modules to the Image "
+                            f"used by `{info.function_name}`:\n"
+                        )
+                        + ", ".join(sorted(warn_missing_modules))


Nit: can we make this a bulleted list? It looks visually strange for a single module imo:

╭─ Modal Deprecation Warning (2024-01-31) ───────────────────────────────────────────────────────────────╮ │ Automatic adding of local python source will be deprecated in the future. │ │ Make sure you have explicitly added the source for the following modules to the Image used by `f`: │ │ helper │ │ │ │ e.g.: │

mwaskom · 2025-01-31T15:12:41Z

modal/_functions.py

+                        + "\n\n"
+                        + (
+                            "e.g.:\n"
+                            f"image = Image.debian_slim().add_local_python_source"


I'm a little worried about this tripping up an overly-literal user who has an Image based on something other than .debian_slim(). I don't know that we have a great way to show a more app-tuned suggestion though since we don't track the Image constructor that was used in an explicit way.

mwaskom · 2025-01-31T15:12:52Z

modal/_functions.py

+                    deprecation_warning(
+                        (2024, 1, 31),
+                        (
+                            "Automatic adding of local python source will be deprecated in the future.\n"


Yeah I think that include_source is not relevant at this stage of the deprecation process; advising users on how to update their Image definitions

mwaskom · 2025-01-31T15:15:30Z

modal/_functions.py

+                            "e.g.:\n"
+                            f"image = Image.debian_slim().add_local_python_source"
+                            f"({python_stringified_modules})\n"
+                            f"@app.function(image=image)\n"


Likewise I'm a little worried about a literal-minded user thinking this change requires the Image to be passed directly to the function (and not the App constructor)

I think it's fine for the suggestion just to demonstrate the Image construction syntax and now how you'd use it.

mwaskom · 2025-01-31T15:16:34Z

modal/_functions.py

+                            f"({python_stringified_modules})\n"
+                            f"@app.function(image=image)\n"
+                            f"..."
+                        ),


Can we add a line like "For more information, see https://modal.com/docs/guide/modal-1-0-migration"? When users start seeing all the new deprecation warnings I think it will be helpful to call their attention to the idea that this is "one last hurrah" to manage their annoyance :D.

We can also add a more verbose explanation of what's going on and the various options for ameliorating it there. I can work on that.

mwaskom · 2025-01-31T15:19:00Z

modal/_functions.py

+                    deprecation_warning(
+                        (2024, 1, 31),
+                        (
+                            "Automatic adding of local python source will be deprecated in the future.\n"


Suggested rephrasing of the prose:

Modal will stop implicitly adding local Python modules to the Image ("automounting") in a future update. The following modules need to be explicitly added for future compatibility:

mwaskom · 2025-01-31T15:20:02Z

modal/image.py

+        img.force_build = force_build
+        return img
+
+    def _extend(self, **kwargs) -> "_Image":


Is there some reason Image._from_args can't automatically transfer the added source set from the provided base Image?

freider added 9 commits December 16, 2024 14:21

Adds warnings if automounts seem to be required

304d09d

Merge remote-tracking branch 'origin/main' into freider/missing-packa…

956c439

…ge-mount-warnings

Fix

631fd54

Adds test etc

a887cf1

comment

0dbac39

Change automounting logic to exclude entrypoint package from auto-mou…

0cf5340

…nts (since it is added by another path)

Remove prints

261932b

Merge remote-tracking branch 'origin/main' into freider/missing-packa…

0315a22

…ge-mount-warnings

Copyright

3a5fcae

freider marked this pull request as ready for review January 10, 2025 15:48

freider changed the title ~~Draft: Adds warnings if automounts seem to be required~~ Adds warnings if automounts seem to be required Jan 13, 2025

freider added 2 commits January 17, 2025 09:44

Settings refactor wip

d3d0a42

Merge remote-tracking branch 'origin/main' into freider/auto-add-conf…

1f04e22

…iguration

freider added 14 commits January 22, 2025 16:20

Simplify

8e14192

Merge remote-tracking branch 'origin/main' into freider/auto-add-conf…

3e99220

…iguration

Fixed test

b74ac27

Update test assertion to reflect new behavior with automount=0

f2f92c1

Change to enum.Enum

859859e

Fix circular import

35d1f0a

Merge remote-tracking branch 'origin/main' into freider/auto-add-conf…

a57bf58

…iguration

Fix unnecessary comparison

0573ece

Fix for config only working with eval()-values

e6da2fd

add type ignore for __notes__

a2a7647

Test assertion

14db34f

Merge branch 'freider/auto-add-configuration' into freider/missing-pa…

6f7e792

…ckage-mount-warnings

Updated message

68584cc

Updated values of include_source attribute

498c947

freider added 11 commits January 27, 2025 11:28

types

f5190b6

Merge branch 'freider/auto-add-configuration' into freider/missing-pa…

e39d6b2

…ckage-mount-warnings

Remove comment

b3dc36b

Blah

5c48fc5

move

0851be3

Restore automount config parsing

ce7e700

port logic

18f30a5

Updated error message

8d592a9

Merge branch 'freider/auto-add-configuration' into freider/missing-pa…

0236192

…ckage-mount-warnings

Bump minor version

bf2b590

Merge branch 'freider/auto-add-configuration' into freider/missing-pa…

9284f1c

…ckage-mount-warnings

freider changed the base branch from main to freider/auto-add-configuration January 27, 2025 12:53

freider added 4 commits January 28, 2025 13:35

Don't accept legacy as an explicit value

10fe5dc

Change back to including non-py-files in the entrypoint package mount

72d5f23

Comment

e0b223e

Merge branch 'freider/auto-add-configuration' into freider/missing-pa…

a48bfbf

…ckage-mount-warnings

Base automatically changed from freider/auto-add-configuration to main January 30, 2025 08:27

freider added 2 commits January 30, 2025 09:53

Merge remote-tracking branch 'origin/main' into freider/missing-packa…

fb7db5b

…ge-mount-warnings

Fix tests

b084a55

freider requested a review from mwaskom January 30, 2025 12:57

freider commented Jan 30, 2025

View reviewed changes

freider added 3 commits January 31, 2025 11:13

Reapply changes to _functions

ec0ad9a

Apply functions from main

7384abf

Merge remote-tracking branch 'origin/main' into freider/missing-packa…

2f962db

…ge-mount-warnings

freider commented Jan 31, 2025

View reviewed changes

Update warning text

341abc4

freider commented Jan 31, 2025

View reviewed changes

mwaskom reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds warnings if automounts seem to be required #2667

Adds warnings if automounts seem to be required #2667

freider commented Dec 16, 2024 •

edited

Loading

mwaskom commented Jan 14, 2025

freider commented Jan 22, 2025

mwaskom commented Jan 22, 2025 •

edited

Loading

freider Jan 30, 2025

freider Jan 30, 2025

mwaskom Jan 31, 2025

freider Jan 31, 2025

freider Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

mwaskom Jan 31, 2025

Adds warnings if automounts seem to be required #2667

Are you sure you want to change the base?

Adds warnings if automounts seem to be required #2667

Conversation

freider commented Dec 16, 2024 • edited Loading

Changelog

mwaskom commented Jan 14, 2025

freider commented Jan 22, 2025

mwaskom commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freider commented Dec 16, 2024 •

edited

Loading

mwaskom commented Jan 22, 2025 •

edited

Loading