refactor(framework): Create SuperLinkLifespanConfig#7497
Open
danieljanes wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the flower-superlink CLI startup flow by introducing a SuperLinkLifespanConfig dataclass and extracting CLI parsing/validation into a dedicated helper, with tests to ensure “final defaults” are applied consistently.
Changes:
- Added
SuperLinkLifespanConfigand_parse_superlink_lifespan_config()to centralize CLI parsing, validation, and derived defaults (e.g., Fleet API default address). - Updated
flower_superlink()to consume the parsed config instead of reading directly from argparseargs. - Added unit tests covering the new config parsing behavior (final defaults + deprecated
--exec-api-addressmapping).
Critical issues
- None found.
Simplicity/readability suggestions
- Replace
cast(str, config.fleet_api_address)with an assertion to encode the invariant and avoid relying on a type-cast (comment added inline).
Consistency concerns
- A module-level
# pylint: disable=too-many-linesweakens linting as the file grows; consider splitting CLI parsing/config/startup into smaller modules rather than globally disabling the check.
Whether the PR should be split
- Not required, but if follow-ups are planned, consider a separate PR to split
flower_superlink.pyinto smaller files to remove the global pylint suppression.
Brief overall verdict
- The refactor is directionally sound and tests help, but there’s a behavioral change around when the “Starting” log/telemetry event fires that should be confirmed as intended.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| framework/py/flwr/superlink/cli/flower_superlink.py | Extracts CLI parsing into _parse_superlink_lifespan_config() and introduces SuperLinkLifespanConfig to drive server startup. |
| framework/py/flwr/superlink/cli/flower_superlink_test.py | Adds tests verifying config parsing applies derived defaults and preserves deprecated address mapping behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ============================================================================== | ||
| """`flower-superlink` command.""" | ||
|
|
||
| # pylint: disable=too-many-lines |
Comment on lines
+451
to
+455
| config = _parse_superlink_lifespan_config() | ||
|
|
||
| log(INFO, "Starting Flower SuperLink") | ||
|
|
||
| event(EventType.RUN_SUPERLINK_ENTER) |
Comment on lines
497
to
+499
| if not is_simulation: | ||
| if not args.fleet_api_address: | ||
| if args.fleet_api_type in [ | ||
| TRANSPORT_TYPE_GRPC_RERE, | ||
| TRANSPORT_TYPE_GRPC_ADAPTER, | ||
| ]: | ||
| args.fleet_api_address = FLEET_API_GRPC_RERE_DEFAULT_ADDRESS | ||
| elif args.fleet_api_type == TRANSPORT_TYPE_REST: | ||
| args.fleet_api_address = FLEET_API_REST_DEFAULT_ADDRESS | ||
|
|
||
| fleet_address, host, port = _format_address(args.fleet_api_address) | ||
|
|
||
| num_workers = args.fleet_api_num_workers | ||
| fleet_address, host, port = _format_address(cast(str, config.fleet_api_address)) | ||
|
|
github-actions
Bot
added
the
Maintainer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.