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

Add App.from_name for construction of a lazily-hydratable App #2798

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

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented Jan 23, 2025

Describe your changes

This PR adds a new App.from_name method with similar lazy construction semantics to other .from_name methods on Object subclasses, along with an App.hydrate method for on-demand hydration. The intention is to make it possible to deprecate App.lookup when we deprecate other Object.lookup methods, streamlining object construction onto a single lazy path.

As App is not a subclass of Object we face a decision about whether we should make it one (and leverage the existing hydration plumbing) or add some App-specific plumbing. This PR takes the latter approach, which feels more straightforward now without foreclosing on adapting the other approach later.

There's definitely some confusing things here. I've made it so that you can't call App.hydrate if you instantiated an App by calling its main constructor (App(name)). Potentially we will want to leverage lazy-hydration of the App layout when we do App redeployments (i.e. the current _init_local_app_existing). I think we can punt on that as there are not currently any user-facing workflows where you would want to hydrate the App and then access its layout, but we've tlaked about it, e.g. for Function discovery after looking up a deployed App.

Part of CLI-96

Backward/forward compatibility checks

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

  • Added a new App.from_name methods for lazily constructing an App based on a lookup and an App.hydrate method for hydrating it with server metadata on demand. These methods should be used instead of the eager App.lookup method, which will be deprecated soon (along with other .lookup methods). Note that App.from_name is primarily useful when you need an App to associate with a modal.Sandbox; other use-cases should continue to create Apps via the main constructor (app = App(name)).

@mwaskom mwaskom changed the title Add App.from_name for contrsuction of a lazy-hydratable App Add App.from_name for construction of a lazyily-hydratable App Jan 23, 2025
self._running_app = RunningApp(response.app_id, interactive=False)

app = _App(name)
app._load = _load
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super minor but this could maybe just be a constructor argument

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m fairly averse to “private parameters” in public interfaces: best case, you confuse users or at least present them with information they don’t need to think about, worse case they try to use it and end up in trouble somehow.

Copy link
Contributor Author

@mwaskom mwaskom Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d really prefer to just merge __init__ and .from_name but couldn’t see a simple way to do that.

@mwaskom mwaskom changed the title Add App.from_name for construction of a lazyily-hydratable App Add App.from_name for construction of a lazily-hydratable App Jan 23, 2025
@freider
Copy link
Contributor

freider commented Jan 24, 2025

What would be the behavior with this if a user does the seemingly allowed (from a typing perspective):

app = modal.App.from_name("blah", create_if_missing=True)

@app.function()
def foo():
    pass

app.remote()

?

Maybe from_name shouldn't actually return an App, but a more limited smaller subset of its protocol (with every method except function and cls? (Or we should make sure that @app.function and @app.cls raise good exceptions for lazy-created apps)

Thinking some more about this it feels like the whole App concept could use some explanation now that we've removed everything app-associated except functions and apps (and sandboxes likely getting uncoupled from apps too soon?)

The core part of an app feels like it's an "atomic group for service deployment [and log collection]"? (with deployment in both the ephemeral and persistent sense)

Am I correct that the only reason we even have the lookup/from_name methods on App is for the sandbox usecase, since those currently require apps?

@mwaskom
Copy link
Contributor Author

mwaskom commented Jan 25, 2025

What would be the behavior with this if a user does the seemingly allowed

This is currently (i.e. with App.lookup) poorly defined and a little surprising: https://linear.app/modal-labs/issue/CLI-79/define-and-enforce-rules-about-what-you-can-do-with-looked-up-apps

Am I correct that the only reason we even have the lookup/from_name methods on App is for the sandbox usecase, since those currently require apps?

Yes I think that's currently correct. In practice, Sandbox.create just needs an app_id. If we expand the App API to support more programmatic discovery of a deployed App layout (which seems like a good idea) then you could also imagine a lookup operation being useful for that in the future.

Maybe from_name shouldn't actually return an App, but a more limited smaller subset of its protocol

That's an interesting idea. Feels like it potentially introduces some complications? E.g. currently with Sandbox.create you can .lookup a (deployed) App and pass it to app=, but you can also pass an ephemeral App that's in a running state. So we'd have something like app: App | AppView. We'd achieve a distinction from a strict typing perspective, but there still might be a lot of conceptual confusion.

Personally I think the situation with App() vs. App.lookup() is a little unfortunate, but not sure what the best thing to do would be. I think the big-picture question here are orthogonal to the specific need to introduce a lazy complement to App.lookup though.

@erikbern
Copy link
Contributor

Open to revisiting app-less sandboxes if this is a big can of worms!

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