fix(framework): Fix flwr no args update check#6939
fix(framework): Fix flwr no args update check#6939mohammadnaseri wants to merge 4 commits intomainfrom
flwr no args update check#6939Conversation
mohammadnaseri
requested review from
danieljanes,
jafermarq,
panh99 and
tanertopal
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc14a54a30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| @app.callback(invoke_without_command=True) | ||
| def main( | ||
| ctx: typer.Context, |
There was a problem hiding this comment.
Adding a required ctx parameter breaks direct invocations of main outside Typer’s command runner, which now raises TypeError before the callback logic runs. In this repo, framework/py/flwr/cli/cli_test.py calls app_module.main(version=False) directly to verify update-check behavior, so this change introduces an immediate regression for those call paths unless ctx is made optional or those callers are updated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Fixes the flwr CLI startup behavior so that invoking flwr with no subcommand can still run the update-check logic while showing help output.
Changes:
- Removed
no_args_is_helpfrom the Typer app configuration and added explicit “no args” help handling in the callback. - Updated the CLI callback signature to accept a Typer/Click context and used it to render help.
- Adjusted CLI tests to construct a Click context and pass it into
main().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| framework/py/flwr/cli/app.py | Reworks no-args behavior by printing help in the callback (after update check) and exiting. |
| framework/py/flwr/cli/cli_test.py | Updates tests to provide a Click context to the callback and imports the Click command object. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if ctx.invoked_subcommand is None and len(sys.argv) == 1: | ||
| typer.echo(ctx.get_help()) | ||
| raise typer.Exit(code=2) |
There was a problem hiding this comment.
The new no-args handling prints help and then exits with code=2. Previously no_args_is_help=True would show help without treating it as a usage error; changing the exit code can be a breaking CLI behavior change for scripts. If the intent is still “show help on bare flwr”, consider exiting with code 0 (or otherwise matching the previous Click/Typer behavior).
| raise typer.Exit(code=2) | |
| raise typer.Exit() |
| typer.secho(f"Flower version: {package_version}", fg="blue") | ||
| raise typer.Exit() | ||
|
|
||
| if ctx.invoked_subcommand is None and len(sys.argv) == 1: |
There was a problem hiding this comment.
The no-args detection uses len(sys.argv) == 1, which couples behavior to global process state and can be incorrect for programmatic invocations/tests where sys.argv doesn’t reflect the arguments passed to Typer. Prefer deriving this from the Click/Typer context (e.g., checking whether any args/subcommand were provided via the context) instead of sys.argv.
| if ctx.invoked_subcommand is None and len(sys.argv) == 1: | |
| if ctx.invoked_subcommand is None and not ctx.args and not ctx.params: |
| def test_flwr_callback_checks_for_update(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| """The top-level flwr callback should perform the startup update check.""" | ||
|
|
||
| class _SentinelError(Exception): | ||
| pass | ||
|
|
||
| captured: dict[str, str] = {} | ||
|
|
||
| def _raise_sentinel(process_name: str | None = None) -> None: | ||
| if process_name is not None: | ||
| captured["process_name"] = process_name | ||
| raise _SentinelError() | ||
|
|
||
| monkeypatch.setattr(app_module, "warn_if_flwr_update_available", _raise_sentinel) | ||
|
|
||
| with pytest.raises(_SentinelError): | ||
| app_module.main(version=False) | ||
| app_module.main(_make_context(), version=False) | ||
|
|
||
| assert captured == {"process_name": "flwr"} |
There was a problem hiding this comment.
This PR is intended to fix the “no args” update-check path, but the tests currently exercise main() directly with the test runner’s sys.argv (which won’t be the bare flwr case). Add a test that invokes the CLI with no arguments (e.g., via CliRunner.invoke(app, [])) and asserts the update check is performed (and that help/exit code match the intended behavior).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4707f96baf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| typer.secho(f"Flower version: {package_version}", fg="blue") | ||
| raise typer.Exit() | ||
|
|
||
| if ctx.invoked_subcommand is None and len(sys.argv) == 1: |
There was a problem hiding this comment.
Checking len(sys.argv) == 1 is not a reliable way to detect an empty flwr invocation when the CLI is called programmatically with explicit args (for example via Click/Typer test runners), because Click parses the provided arg list without updating process-wide sys.argv. In those calls, ctx.invoked_subcommand is None but this condition is false, so the callback skips the help/exit path and returns success with no output, which is a behavioral regression from the prior no_args_is_help=True behavior.
Useful? React with 👍 / 👎.
github-actions
bot
added
the
Maintainer
mohammadnaseri
changed the title
flwr no args update checkflwr no args update check
mohammadnaseri
changed the title
flwr no args update checkflwr no args update check
No description provided.