-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat: improve CLI parameter handling and cleanup unused code #4002
Conversation
CLI > specific env_file > default env_file
This pull request is automatically being deployed by Amplify Hosting (learn more). |
...reviewing the unit tests |
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.
LGTM
workers: int = typer.Option(1, help="Number of worker processes.", envvar="LANGFLOW_WORKERS"), | ||
timeout: int = typer.Option(300, help="Worker timeout in seconds.", envvar="LANGFLOW_WORKER_TIMEOUT"), | ||
port: int = typer.Option(7860, help="Port to listen on.", envvar="LANGFLOW_PORT"), | ||
host: str | None = typer.Option(None, help="Host to bind the server to.", show_default=False), |
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.
Not related to this PR but it would probably be nice to use the syntax host: Annotated[str | None, typer.Option(None, help="Host to bind the server to.", show_default=False)]
= None` for all the params for better type checking.
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 tried applying the suggestion, but the code started breaking. I'll take a closer look at it later.
components_path: Path | None = typer.Option( | ||
Path(__file__).parent / "components", | ||
str(Path(__file__).parent / "components"), |
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.
Should components_path
type be changed to str
?
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.
Oops... that was a small oversight. It should be Path
35fa03a
to
2d47df2
Compare
This PR refines CLI parameter handling, adds default path values, enforces parameter hierarchy, and removes unused code for improved maintainability.
Fixes #3820