Make CLI import logic consistent between listings and runnability #2787
Conversation
…ables in a module
|
Is there a typo in the first changelog bullet? I am having trouble parsing it. |
Yup, fixed |
There was a problem hiding this comment.
Definitely a lot here ... had a few questions but overall lgtm; thanks for cleaning this up.
Does this PR also fix https://linear.app/modal-labs/issue/CLI-293/regression-modal-run-against-class-w-lifecycle-methods-doesnt-auto-run?
| app: App, module, accept_local_entrypoint: bool, accept_webhook: bool | ||
| ) -> Union[Function, LocalEntrypoint, MethodReference]: | ||
| """Using only an app - automatically infer a single "runnable" for a `modal run` invocation | ||
| Runnables includes all Functions, (class) Methods and Local Entrypoints, including web endpoints. |
There was a problem hiding this comment.
Why are web endpoints considered "runnable"?
There was a problem hiding this comment.
modal shell can "run" them - happy to consider other names (but I'm only using this internally at the moment)
There was a problem hiding this comment.
Ah interesting, yeah I guess that makes sense.
modal/cli/import_refs.py
Outdated
| ] | ||
| apps = cast(list[tuple[str, App]], inspect.getmembers(module, lambda x: isinstance(x, App))) | ||
|
|
||
| all_runnables: dict[Runnable, list[str]] = {} |
There was a problem hiding this comment.
Would using defaultdict make some of the updates marginally simpler?
modal/cli/import_refs.py
Outdated
| f"Expected to find an app variable named **`{DEFAULT_APP_NAME}`** (the default app name). " | ||
| "If your `modal.App` is named differently, " |
There was a problem hiding this comment.
Something I'm a little confused by: is this guiding users to to the App name (e.g. App("my-app")) or to the variable name that points at the App?
There was a problem hiding this comment.
It's supposed to be the variable name, not the name of the app (this is unchanged from before, it's just that any references via the app has been broken since we deprecated App.__getattr__ 😬 )
There was a problem hiding this comment.
Maybe could rephrase (e.g. "If your modal.App is assigned to a different variable name") to reduce the chance of confusion 🤷♂️
Oooh, good question. Not intentionally, but seems very relevant so I should make it so if I haven't! (It would be a bug if it listed/could run the lifecycle methods) |
Seems like this patch fixed it, since it extracts the method names using the method metadata on the class. I made sure to add a lifecycle method now to one of the tests to keep this from regressing! |
Changes the CLI import logic to use an exhaustive list of commands + filters instead of
getattr()navigation.This ensures that the listed modal "runnables" in the CLI error output will always be runnable by the command that you've run, which wasn't previously the case.
While this was a lot of code, I think it's mostly a simplification of the previous code.
The tests are still a bit of a mess since I mostly ported the old tests, and they now overlap quite a lot, but they are quick so it doesn't matter too much.
Note that this does remove a few of the helpful commands for special cases that we had before in favor of a more general error message that always lists the runnable commands if the user passes something that can't resolve to a single "runnable" (function, entrypoint, method)
Also fixes CLI-293
Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Changelog
mymodule::{app_variable}.{function_name}.modal run,modal serveandmodal shellcommands get more consistent error messages in cases where the passed app or function reference isn't resolvable to something that the current command expects.__getattr__,__setattr__,__getitem__and__setitem__methods frommodal.App