refactor(cli): Fail fast deprecated long-running flwr-* wrappers#6953
refactor(cli): Fail fast deprecated long-running flwr-* wrappers#6953
Conversation
…or.py Co-authored-by: Heng Pan <pan@flower.ai>
…or.py Co-authored-by: Heng Pan <pan@flower.ai>
…entappio-auth-interceptors
…entappio-auth-interceptors
…entappio-auth-interceptors
…entappio-auth-interceptors
…entappio-auth-interceptors
There was a problem hiding this comment.
Pull request overview
This PR enforces fail-fast behavior for deprecated long-running flwr-* wrapper commands, directing users to migrate to flower-superexec.
Changes:
- Replaced
run_with_deprecation_warning(...)long-running wrapper behavior with immediateflwr_exit(...)for missing--token. - Removed now-unused SuperExec-related imports (
ExecPluginType, stubs, plugins, andrun_with_deprecation_warning) from the affected entrypoints. - Added migration messaging pointing users to
flower-superexec --plugin-type ....
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| framework/py/flwr/supernode/cli/flwr_clientapp.py | Fail-fast when invoked without token (deprecated long-running wrapper path). |
| framework/py/flwr/simulation/app.py | Fail-fast when invoked without token; remove deprecated wrapper plumbing. |
| framework/py/flwr/server/serverapp/app.py | Fail-fast when invoked without token; remove deprecated wrapper plumbing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flwr_exit( | ||
| ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG, | ||
| "Long-running mode in `flwr-clientapp` is no longer supported. " | ||
| "Use `flower-superexec --plugin-type clientapp` instead.", | ||
| ) |
There was a problem hiding this comment.
ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG appends short-help text about an invalid SuperExec YAML plugin config, which is misleading for this fail-fast path (missing --token/deprecated long-running wrapper usage). Consider using a more appropriate exit code, or introduce a dedicated exit code/help entry for deprecated-wrapper/unsupported long-running mode so the rendered short-help and docs URL match the actual error.
| flwr_exit( | ||
| ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG, | ||
| "Long-running mode in `flwr-simulation` is no longer supported. " | ||
| "Use `flower-superexec --plugin-type serverapp` instead.", | ||
| ) |
There was a problem hiding this comment.
ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG will also print the short-help message "The YAML configuration for the SuperExec plugin is invalid.", which doesn’t align with the actual error here (deprecated long-running flwr-simulation invocation without --token). Please switch to a more fitting exit code or add a dedicated exit code/help entry for this deprecated-wrapper policy so users aren’t pointed at the wrong docs.
| flwr_exit( | |
| ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG, | |
| "Long-running mode in `flwr-simulation` is no longer supported. " | |
| "Use `flower-superexec --plugin-type serverapp` instead.", | |
| ) | |
| log( | |
| ERROR, | |
| "Long-running mode in `flwr-simulation` is no longer supported. " | |
| "Use `flower-superexec --plugin-type serverapp` instead.", | |
| ) | |
| restore_output() | |
| raise SystemExit(1) |
| flwr_exit( | ||
| ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG, | ||
| "Long-running mode in `flwr-serverapp` is no longer supported. " | ||
| "Use `flower-superexec --plugin-type serverapp` instead.", | ||
| ) |
There was a problem hiding this comment.
Same issue as in other wrappers: using ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG will append short-help text about invalid SuperExec YAML config, which is confusing for the unsupported long-running flwr-serverapp mode (missing --token). Consider a dedicated exit code/help entry for deprecated wrapper usage or another exit code that better matches this condition.
| flwr_exit( | ||
| ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG, | ||
| "Long-running mode in `flwr-clientapp` is no longer supported. " | ||
| "Use `flower-superexec --plugin-type clientapp` instead.", |
There was a problem hiding this comment.
The suggested migration command (flower-superexec --plugin-type clientapp) isn’t runnable as-is: flower-superexec also requires --appio-api-address and one of --superexec-auth-secret-file/--superexec-auth-secret-stdin (and typically --insecure, given the earlier TLS check). Please update the message to include the minimum required flags or point users to a specific docs page/example command so the guidance is actionable.
| "Use `flower-superexec --plugin-type clientapp` instead.", | |
| "Use `flower-superexec --plugin-type clientapp " | |
| "--appio-api-address <ADDRESS> " | |
| "--superexec-auth-secret-file <PATH> --insecure` instead.", |
| flwr_exit( | ||
| ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG, | ||
| "Long-running mode in `flwr-simulation` is no longer supported. " | ||
| "Use `flower-superexec --plugin-type serverapp` instead.", |
There was a problem hiding this comment.
The migration hint flower-superexec --plugin-type serverapp is incomplete: flower-superexec requires --appio-api-address and a SuperExec auth secret flag (--superexec-auth-secret-file or --superexec-auth-secret-stdin), and will also require --insecure in the current TLS-less implementation. Consider expanding the message to a minimal working command or linking to the relevant docs so users can follow it successfully.
| "Use `flower-superexec --plugin-type serverapp` instead.", | |
| "Use `flower-superexec --plugin-type serverapp " | |
| f"--appio-api-address {args.serverappio_api_address} " | |
| "--superexec-auth-secret-file <path> --insecure` instead " | |
| "(or replace `--superexec-auth-secret-file <path>` with " | |
| "`--superexec-auth-secret-stdin`).", |
| flwr_exit( | ||
| ExitCode.SUPEREXEC_INVALID_PLUGIN_CONFIG, | ||
| "Long-running mode in `flwr-serverapp` is no longer supported. " | ||
| "Use `flower-superexec --plugin-type serverapp` instead.", |
There was a problem hiding this comment.
The guidance Use flower-superexec --plugin-type serverapp instead. omits required flower-superexec args (--appio-api-address and one of --superexec-auth-secret-file/--superexec-auth-secret-stdin, plus --insecure given current TLS limitations). Please adjust the message to include a minimal working invocation or a docs link so the next step is clear and immediately usable.
| "Use `flower-superexec --plugin-type serverapp` instead.", | |
| "Use `flower-superexec --plugin-type serverapp " | |
| "--appio-api-address <APPIO_API_ADDRESS> " | |
| "--superexec-auth-secret-file <PATH_TO_SECRET> " | |
| "--insecure` instead.", |
6b386c0 to
e111e40
Compare
Issue
Description
Part 6/6 of the SuperExec auth stack. This PR is intentionally scoped so reviewers can focus on one or two concepts at a time.
Related issues/PRs
appio-authn/2-and-3-wire-serverappio-and-clientappio-auth-interceptorsappio-authn/pr-5-clientappio-superexec-wiringappio-authn/pr-6-deprecated-wrapper-policyappio-authn/pr-5-clientappio-superexec-wiringappio-authn/pr-6-deprecated-wrapper-policy-testsProposal
Explanation
This PR delivers one reviewable slice of the SuperExec shared-secret auth rollout.
Review focus for this PR:
flower-superexecReviewer notes:
appio-authn/pr-6-deprecated-wrapper-policy-testsChecklist
#contributions)Any other comments?
This PR is part of a review-focused stacked rollout. If preferred, I can squash/reorder after initial review passes.