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

Allow app.run inside of functions #2883

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

freider
Copy link
Contributor

@freider freider commented Feb 20, 2025

In the same vein as #2882

The intent of it was to prevent usage of app.run in global scope of modules that would then get loaded in containers causing an infinite recursion. This is not as likely to happen anymore now that we have the modal run cli instead of if __name__ == __main__ as the main app launcher mechanic

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

  • Allow running new ephemeral apps from within Modal containers using with app.run(): .... Use with care, as putting such a run block in global scope of a module could easily lead to infinite app creation recursion

…ounts from within running functions

iirc, the checks were mostly in place to prevent unnecessary logic from running in global scope of containers,
but just adding pre-defined mounts to a list wouldn't account for many clock cycles, and this change still
prevents auto-mounting from running within containers (soon to be deprecated anyways)
The intent of it was to prevent usage of app.run in global scope of modules that
would then get loaded in containers causing an infinite recursion. This is not as
likely to happen anymore now that we have the `modal run` cli instead of
`if __name__ == __main__` as the main app launcher mechanic
@mwaskom
Copy link
Contributor

mwaskom commented Feb 20, 2025

This is not as likely to happen anymore now that we have the modal run cli

One thing that I might be missing is why the error message mentions modal run if the existence of modal run obviates the error message. But I'm on board with dropping this guardrail.

Base automatically changed from freider/remove-mount-is-local-checks to main February 20, 2025 17:19
@freider
Copy link
Contributor Author

freider commented Feb 21, 2025

This is not as likely to happen anymore now that we have the modal run cli

One thing that I might be missing is why the error message mentions modal run if the existence of modal run obviates the error message. But I'm on board with dropping this guardrail.

I think it was intended to nudge people towards using modal run instead of having with app.run(): blocks in their app files, as in "using modal run instead will prevent this kind of mistake"

@mwaskom
Copy link
Contributor

mwaskom commented Feb 21, 2025

Does this need a rebase?

@erikbern
Copy link
Contributor

We could in theory set some global variable during code imports to prevent this case more selectively. Not super important though.

@freider
Copy link
Contributor Author

freider commented Feb 24, 2025

We could in theory set some global variable during code imports to prevent this case more selectively. Not super important though.

Good idea, I just applied that to the PR

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.

3 participants