refactor(framework): Implement storing/fetching of SimulationConfig from federation manager#6829
refactor(framework): Implement storing/fetching of SimulationConfig from federation manager#6829
SimulationConfig from federation manager#6829Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Daniel J. Beutel <daniel@flower.ai>
…nt-store-fetch-simulation-config
jafermarq
requested review from
danieljanes,
panh99 and
tanertopal
as code owners
github-actions
bot
added
the
Maintainer
…nt-store-fetch-simulation-config
There was a problem hiding this comment.
Pull request overview
This PR wires SimulationConfig through the federation management layer so simulation federations can persist and surface their runtime configuration (via Control API and CLI), instead of treating it as ephemeral/request-only data.
Changes:
- Extend
Federation(proto + Python typing) to carry aSimulationConfig. - Add
get_simulation_config/set_simulation_configtoFederationManagerand implement inNoOpFederationManagerwith in-memory storage + tests. - Update ControlServicer/CLI to set, fetch, and display/serialize the simulation config.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
framework/py/flwr/superlink/servicer/control/control_servicer.py |
Adds config to ShowFederation response and persists config via ConfigureSimulationFederation. |
framework/py/flwr/superlink/federation/noop_federation_manager.py |
Stores/returns SimulationConfig in-memory for the NoOp federation manager. |
framework/py/flwr/superlink/federation/noop_federation_manager_test.py |
Adds tests for default/stored config behavior and copy semantics. |
framework/py/flwr/superlink/federation/federation_manager.py |
Introduces abstract API for getting/setting simulation config per federation. |
framework/py/flwr/common/typing.py |
Extends Federation dataclass to include SimulationConfig. |
framework/py/flwr/cli/federation/ls.py |
Displays simulation config for simulation federations and adds JSON serialization for config. |
framework/proto/flwr/proto/federation.proto |
Adds SimulationConfig config = 8 to Federation. |
framework/py/flwr/proto/federation_pb2.py |
Regenerated protobuf Python output for new Federation.config field. |
framework/py/flwr/proto/federation_pb2.pyi |
Regenerated typing stubs to reflect Federation.config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
framework/py/flwr/superlink/federation/noop_federation_manager.py
Outdated
Show resolved
Hide resolved
| class Federation: # pylint: disable=R0902 | ||
| """Federation details.""" | ||
|
|
||
| name: str | ||
| description: str | ||
| members: list[Member] | ||
| nodes: list[NodeInfo] | ||
| runs: list[Run] | ||
| archived: bool = False | ||
| simulation: bool = False | ||
| archived: bool | ||
| simulation: bool | ||
| config: SimulationConfig |
There was a problem hiding this comment.
Changing Federation.archived/simulation from defaulted fields to required fields and adding required config is a breaking change for callers constructing flwr.common.typing.Federation (external users may rely on the previous defaults). Consider keeping the previous defaults (archived: bool = False, simulation: bool = False) and using a default_factory for config so existing call sites continue to work while still providing a sensible default config.
framework/py/flwr/superlink/federation/noop_federation_manager_test.py
Outdated
Show resolved
Hide resolved
framework/py/flwr/superlink/federation/noop_federation_manager.py
Outdated
Show resolved
Hide resolved
| def _get_effective_simulation_config(self) -> SimulationConfig: | ||
| """Return the stored simulation configuration or the default copy.""" | ||
| config = SimulationConfig() | ||
| config.CopyFrom(self._simulation_config or DEFAULT_SIMULATION_CONFIG) | ||
| return config |
There was a problem hiding this comment.
This looks like a runtime hack. Should we set self._simulation_config to DEFAULT_SIMULATION_CONFIG in .create_federation method if simulation=True?
There was a problem hiding this comment.
yes, that's a better approach
Co-authored-by: Heng Pan <pan@flower.ai>
No description provided.