-
Notifications
You must be signed in to change notification settings - Fork 106
feat: separate out remaining dependencies and improve tests #789
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| """Typer command definition for ``m serve``. | ||
|
|
||
| Separates the CLI interface (typer annotations) from the server implementation | ||
| (FastAPI, uvicorn) so that ``m --help`` works without the ``server`` extra installed. | ||
| The heavy server dependencies are only imported when ``m serve`` is actually invoked. | ||
| """ | ||
|
|
||
| import typer | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is now an optional dependency if the cli bundle is installed. As such I think it should have a guard/raise an error
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Running any of the Is that what you are referencing? The subcommands only get called / imported if used by |
||
|
|
||
|
|
||
| def serve( | ||
| script_path: str = typer.Argument( | ||
| default="docs/examples/m_serve/example.py", | ||
| help="Path to the Python script to import and serve", | ||
| ), | ||
| host: str = typer.Option("0.0.0.0", help="Host to bind to"), | ||
| port: int = typer.Option(8080, help="Port to bind to"), | ||
| ): | ||
| """Serve a FastAPI endpoint for a given script.""" | ||
| from cli.serve.app import run_server | ||
|
|
||
| run_server(script_path=script_path, host=host, port=port) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typer is in the new cli dependency group. So currently we'd need server and cli (unless typer is transitively found?). Maybe the dependency is needed in server group too, or this message is modified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - I think it is transitive? I wonder if it would then be clearer in the code to split into two and keep typer aligned with a cli exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included the
cligroup in theservergroup that is required here. Technically, it already is covered in the same way as above. You can't actually reachm serveand get this error unless you've already installedm[cli]and worked past that error.