🌱 Upgrade FL controller Flower path to 1.26.x SuperLink/SuperNode architecture - #107
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yanmxa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| ### Deploy a Federated Learning Instance (Flower) | ||
|
|
||
| #### 1. Deploy a Federated Learning Instance | ||
| #### 1. Create a FederatedLearning Resource |
There was a problem hiding this comment.
| #### 1. Create a FederatedLearning Resource | |
| #### 1. Create a Federated Learning Resource |
Should there be a space between FederatedLearning?
There was a problem hiding this comment.
No, the resource name is FederatedLearning
| ListenerPort int | ||
| CreateService bool | ||
| ObsSidecarImage string | ||
| // FlowerServerAppParams defines the parameters for a Flower 2.x SuperExec-ServerApp Deployment on hub. |
There was a problem hiding this comment.
Do you mean 1.2x instead of 2.x? The latest Flower version as of today is 1.26.1.
There was a problem hiding this comment.
Good Catch! You're right, the current Flower version is 1.26.x. I used "2.x" to conceptually distinguish the new SuperLink/SuperNode architecture from the old server/client model, but it's misleading. Will update all "2.x" references to reflect the actual version.
| ObsSidecarImage string | ||
| // FlowerServerAppParams defines the parameters for a Flower 2.x SuperExec-ServerApp Deployment on hub. | ||
| type FlowerServerAppParams struct { | ||
| Namespace string // FL CR namespace (where Deployment is created) |
There was a problem hiding this comment.
Should we use the full form of FL CR so that it's clear?
| // FlowerClientAppParams defines the parameters for a Flower 2.x SuperExec-ClientApp ManifestWorkReplicaSet. | ||
| type FlowerClientAppParams struct { | ||
| Name string // e.g., "<cr-name>-clientapp" | ||
| Namespace string // FL CR namespace (where MWRS is created) |
| return nil | ||
| } | ||
|
|
||
| // pruneOpenFLClientResources cleans up OpenFL client resources (per-cluster ManifestWorks + Placement). |
There was a problem hiding this comment.
Is this an alternate function used only for OpenFL (not Flower)?
There was a problem hiding this comment.
Yes, exactly. pruneClientResources (line 226) dispatches by framework: Flower calls pruneFlowerClientResources, OpenFL calls pruneOpenFLClientResources. This split was needed because Flower now uses ManifestWorkReplicaSet while OpenFL uses per-cluster ManifestWorks — different resources require different cleanup logic.
| return err | ||
| } | ||
|
|
||
| clientParams := &manifests.OpenFLClientParams{ |
There was a problem hiding this comment.
Might be related to my comment above, is this part of the OpenFL function? If it doesn't impact the usage of Flower here, maybe this should go into another PR and focus only on the changes for Flower here.
There was a problem hiding this comment.
To clarify further: the OpenFL code logic is unchanged — only the switch wrapper was removed since it had a single remaining case after extracting Flower. This is already isolated in commit 49a3955. Splitting it into a separate PR would introduce an awkward intermediate state (a single-case switch).
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| // reconcileFlower handles the Flower 2.x SuperLink/SuperNode architecture. |
There was a problem hiding this comment.
See my comment above about Flower version.
| } | ||
|
|
||
| render, deployer := applier.NewRenderer(serverFS), applier.NewDeployer(r.Client) | ||
| serverParams := &manifests.OpenFLServerParams{ |
There was a problem hiding this comment.
Please see my comment above about maybe moving this to another PR.
There was a problem hiding this comment.
Same reasoning as the client file — see my reply here.
|
|
||
| ## Architecture | ||
|
|
||
| ### Flower 2.x (SuperLink/SuperNode) |
There was a problem hiding this comment.
Please see my earlier comment about the Flower version.
There was a problem hiding this comment.
Already updated to 1.26.x.
| --app <your-flower-app> \ | ||
| --federation <superlink-address> |
There was a problem hiding this comment.
| --app <your-flower-app> \ | |
| --federation <superlink-address> | |
| <your-flower-app> \ | |
| <superlink-connection> |
You don't need to pass the --app or --federation flags. They are positional args.
There was a problem hiding this comment.
Done, updated to positional args.
| <summary><strong>Deploy a Federated Learning Instance (OpenFL)</strong></summary> | ||
|
|
||
| #### 2. Schedule the Federated Learning Clients into Managed Clusters | ||
| ### OpenFL Path |
There was a problem hiding this comment.
Could we move the changes for OpenFL to another PR and keep this PR focused only for the Flower-related changes?
|
/retest |
…ture Signed-off-by: Meng Yan <myan@redhat.com>
Remove unnecessary framework switch statements and embed.FS variables since federatedLearningServer() and clusterWorkload() are now only called for the OpenFL path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
The new template files were missed in the initial commit because git add -u only tracks modified/deleted files, not new untracked ones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
…R abbreviation, fix flwr run syntax, split OpenFL docs to separate PR Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
b312554 to
9abaeda
Compare
|
/lgtm |
fcd1224
into
open-cluster-management-io:main
* 🌱 Migrate Flower app-torch example to SuperExec architecture The Flower path in the FederatedLearning controller was upgraded to the 1.26.x SuperLink/SuperNode/SuperExec architecture in #107, but examples/flower/app-torch/ was left on the legacy Flower 1.x `server`/`client` entrypoint. The manifests produced by the controller launch the container with SuperExec args (`--insecure`, `--appio-api-address=<addr>`, `--plugin-type=<serverapp|clientapp>`), which the old entrypoint.sh rejected with `Unsupported command '--insecure'` (see #116). This rewrites the example to match the SuperExec contract used by the flower-addon's cifar10 reference app: - server_app.py / client_app.py now expose `ServerApp` / `ClientApp` objects via `@app.main()` / `@app.train()` / `@app.evaluate()`. - task.py loads MNIST via `flwr_datasets.FederatedDataset` with an `IidPartitioner`; partition-id is derived from the `cluster-name` / `num-partitions` node_config injected by the flower-addon SuperNode. - pyproject.toml pins `flwr>=1.26.1` and registers the new `[tool.flwr.app.components]` entry points. - Dockerfile switches the base image to `flwr/superexec:1.26.1` and bakes PyTorch / Flower deps in at build time. ENTRYPOINT is now `flower-superexec`. This also resolves the slow-startup feedback in issue #116 where the previous image re-downloaded ~2GB of deps on every pod start. - entrypoint.sh and app_torch/utils.py are removed (obsolete with SuperExec). README updates: - Add a step to enable the `ManifestWorkReplicaSet` feature gate on the hub (disabled by default in OCM v1.2.x, per the original report). - Point the sample `FederatedLearning` CR at `quay.io/open-cluster-management/federated-learning-application:flower-mnist-latest` (the legacy `flower-app:cifar10-v1.0.0` image referenced in main is unavailable on quay.io and is incompatible with the SuperExec args). - Clarify the SuperExec image contract and update the `APP_NAME` example to `flower-mnist`. Fixes #116 Signed-off-by: Meng Yan <myan@redhat.com> * Address CodeRabbit review feedback - client_app.py: replace process-randomized `hash()` with `hashlib.sha256` so the cluster-name -> partition-id mapping is stable across pod restarts. - server_app.py: make the final-model save path configurable via the `FLOWER_MODEL_OUTPUT_PATH` env var; skip the save when it's unset (the hub ServerApp Deployment has no PVC by default, so writing to /app was effectively a no-op). - task.py: cache the centralized MNIST DataLoader across rounds and drop the redundant `with_format("torch")` (overridden by `with_transform`). - pyproject.toml: cap `flwr` at `<2` to guard against future breaking major releases. Signed-off-by: Meng Yan <myan@redhat.com> --------- Signed-off-by: Meng Yan <myan@redhat.com>
Related Jira Issue
https://issues.redhat.com/browse/ACM-30551
Summary
Changes
CRD: Added
superlink(default:superlink.flower-system:9091) andsupernode(default:flower-supernode.flower-addon:9094) endpoint fields to ServerSpec/ClientSpecController: New
reconcileFlower()path deploys:Removed: Old Flower 1.x templates (server-job, service, route, client manifestwork) and associated param structs
Added: RBAC for
apps/deploymentsandwork.open-cluster-management.io/manifestworkreplicasets,workv1alpha1scheme registrationTest plan
make generate && make manifests- CRD generation succeeds with new fieldsmake build- Go compilation passesmake test- All existing tests pass🤖 Generated with Claude Code