refactor(fl)!: complete the NVFLARE Client-API migration — spleen tutorial ported, legacy Executor syntax retired, job types renamed, fed_opt migrated

[atriaybagur]

Completes the NVFLARE Client-API migration tracked by #665: ports flip-fl-base#116 (spleen tutorial → Client API), then retires the legacy Executor syntax entirely — the Client-API tutorials, templates and job types take over the plain names, converging NVFLARE's job-type naming with Flower's.

1. Spleen tutorial migrated to the Client API (flip-fl-base#116)

• New fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation (Client API): job.py wires FlipFedAvgRecipe (FLIP server components, analytics bridge, VAL_DICE best-model selection); a single trainer.py replaces the FLIP_TRAINER/FLIP_VALIDATOR executor pair (train + evaluate tasks); metrics ride SummaryWriter tags through FlipAnalyticsBridge; models.py stays checkpoint-compatible with the evaluation tutorial; the trainer fails loudly when no image/label pair is found, naming the data-enrichment step.
• The half-landed vanilla-FedAvgRecipe port (client.py/job.py/model.py at the old tutorial root) is superseded and removed.

2. Legacy Executor tutorials retired; Client-API tutorials renamed to the plain names

• Removed the Executor variants of xray_classification, 3d_spleen_segmentation, 3d_spleen_segmentation_evaluation and latent_diffusion_model; the *_client_api directories were renamed onto the plain names (preserving the legacy dirs' unique assets: xray + spleen bundle-export configs, the spleen utils/ download tooling + pyproject.toml).
• The legacy Docker testing/ harness (app_organiser.sh + compose simulator) and the test-template target are removed — every tutorial now runs via its job.py on SimEnv (make run → make sim) from the flip-utils venv.

3. Job types renamed: *_client_api → the plain names (breaking)

• fl-apps/nvflare/{standard,evaluation,diffusion_model} are now the recipe-generated Client-API templates (legacy Executor templates deleted); the aggregate required_files.json is regenerated — standard no longer requires validator.py, evaluation now requires models.py.
• Runtime aliases are kept for pre-rename models: evaluation_client_api in flip-api's results handling and standard_client_api in flip.export.bundle's exportable set.
• Breaking for external apps: an upload whose config.json still says "job_type": "standard_client_api" / "evaluation_client_api" is now rejected (not in the manifest). In-tree apps (incl. the Ark+ tutorials) are updated; the FLIP-Ark repo's app configs need the same one-line change before the next stag deploy.
• flip-ui demo mocks updated to mirror the new manifest.

4. fed_opt migrated to the Client API too

• New flip.nvflare.recipes.FlipFedOptRecipe (+ FlipFedOptShareableGenerator, stock PTFedOptModelShareableGenerator extended to source the model from the user's models.get_model). This is the layout NVFlare's own Client-API FedOptRecipe (nvflare.app_opt.pt.recipes.fedopt) wires — stock SAG + WEIGHT_DIFF aggregator + the FedOpt shareable generator, with the upstream note "FedOpt is only implemented for TransferType.DIFF and DataKind.WEIGHT_DIFF in the aggregator" — with FLIP's forked SAG and server components swapped in, exactly as FlipFedAvgRecipe relates to upstream's FedAvgRecipe. The client contract is identical to standard, so every standard app runs under fed_opt unchanged; required_files drops validator.py.
• Server-optimizer defaults follow stock NVFLARE FedOpt (SGD lr=1.0, momentum=0.6, no scheduler) rather than the retired template's Adam-at-0.5 — that setting was never live (see the guard bug below) and, once the guard is fixed, demolishes the global model in a single step (verified: round-1 Dice pinned at 0.0000 under it).
• Bug fix in flip.nvflare.controllers.ScatterAndGather: the FedOpt guard in _diff_to_weights tested isinstance(self.aggregator, PTFedOptModelShareableGenerator) — never true for any real config (that class is a shareable generator, not an aggregator) — so the DIFF→WEIGHTS reconstruction ran unconditionally and fed the FedOpt aggregator full weights it then rejected: the legacy fed_opt job type never actually aggregated. The fixed guard keys on the aggregator's expected_data_kind, handling both its shapes (plain DataKind when hand-constructed, the {dxo_name: DataKind} dict it is normalised to at runtime — the subtlety that made the first fix attempt miss). The unit test that mirrored the bug (its stub subclassed the generator to satisfy the isinstance) is rewritten against the realistic dict-form aggregator.
• Note: flip-utils changes reach deployed stacks via the FL image rebuild (make build-fl), the standard flip-utils shipping path.

5. Aggregation aligned with stock NVFLARE: the DIFF→WEIGHTS bridge is gone

Git archaeology (prompted by review discussion) showed ScatterAndGather._diff_to_weights was never a design requirement: it came in with the pre-open-source import as a bridge between DIFF-sending trainers and the legacy templates' WEIGHTS-expecting aggregator, and the later "Fix fed-opt issues" commits tried (and failed — the isinstance guard) to carve FedOpt out of it. Stock NVFLARE needs none of this: the aggregator's stock default expects WEIGHT_DIFF, averages the diffs directly, and FullModelShareableGenerator adds the average onto the global model — mathematically identical for FedAvg (avg(g+dᵢ) = g+avg(dᵢ)) and natively partial-safe for frozen-backbone head-only updates (keys absent from the diff keep their global value — the exact guarantee FLIP#684's reconstruction existed to provide).

• _diff_to_weights is deleted; every template (standard, evaluation, diffusion_model, fed_opt) now wires a bare InTimeAccumulateWeightedAggregator() (stock default) and aggregates client diffs directly. The FedOpt-specific guard becomes unnecessary and is gone with it.
• fl-api's config assembly only ever injects aggregation_weights into the aggregator (never expected_data_kind), so the platform path is untouched.
• Deploy coupling: the new templates and the new flip-utils must ship together — the FL images are rebuilt (make build-fl) whenever flip-utils changes, which is the standard shipping path, so this is the normal release flow rather than an extra step.
• Cosmetic: the last _client_api strings (simulator workspace defaults in the tutorials' job.py/Makefiles and the round-metrics script) are renamed away.
• Follow-on fix caught by running the Ark+ tutorials: both Ark+ evaluation Makefiles passed an explicit --workspace ../../testing/runs/workspace to job.py, pointing into the removed harness dir — the override is dropped so job.py's /tmp/nvflare/<name> default applies, like every other tutorial.

Verification

Unit/static: fl-apps/check_required_files.sh regenerated + clean; ruff clean across the touched trees (the NVFLARE tutorial tree's pre-existing lint debt went away with the legacy files); flip-utils 732 unit tests (incl. new FlipFedOptRecipe tests + the rewritten SAG guard test), flip-api 1501, fl-api-base 149, imaging-api untouched-by-this-PR.

Local simulator (RTX 5090, MSD spleen):

• Renamed spleen tutorial (make -C fl-tutorials run-tutorial TUTORIAL=3d_spleen_segmentation): clean end-to-end run at the committed config.
• FlipFedOptRecipe driven with the spleen tutorial's unmodified app_files (2 rounds): zero aggregator rejections and healthy dynamics — round-0 VAL_DICE 0.012→0.031, a normal post-aggregation dip (0.018) with recovery, final global TEST_DICE 0.0167. Under the old guard the same run logged expected WEIGHT_DIFF but got WEIGHTS and rejected every contribution.
• make export output for the renamed standard template matches the committed configs exactly.

Stock-aggregation verification — every job type, zero aggregator rejections everywhere:

Path	Run	Result
standard	spleen tutorial sim, 2 rounds × 2 sites	✅ final-global TEST_DICE 0.0504
fed_opt	spleen app under FlipFedOptRecipe, 2 rounds	✅ final-global TEST_DICE 0.0609
head-only / frozen-backbone	spleen + aggregate_only_regex, 2 rounds	✅ KeepOnlyVars → partial-diff aggregation → TrimBroadcastVars → ReconstructFullModel all engaged; TEST_DICE 0.0140
diffusion_model	LDM tutorial, 1 AE + 1 DM round × 2 sites	✅ both stages aggregated (18 train_ae + 18 train_dm tasks)
evaluation	Ark+ baseline evaluation tutorial	✅ real per-lesion AUROC output (Effusion ≈1.0)
evaluation (multi-model)	Ark+ multimodel evaluation tutorial	✅ per-model AUROC output
standard + SERVER_CHECKPOINT + head-only	Ark+ fine-tuning tutorial (2.47 GB Swin backbone, AGGREGATE_ONLY_REGEX=omni_heads)	✅ 3 rounds × 2 sites, zero rejections; full filter chain in the exported job (KeepOnlyVars+PercentilePrivacy → TrimBroadcastVars → ReconstructFullModel) over the bare stock aggregator; final per-lesion TEST precision 0.97–1.0 / recall 1.0
Platform e2e (standard)	dev stack on FL images rebuilt from this branch	✅ upload → bundle → train → RESULTS_UPLOADED, final-global val_acc 0.0602 (model 2149843f), best+final checkpoints in the zip, zero rejections on the live fl-server

Dev-stack e2e (platform path, GSTT, NVFLARE backend):

• Original migration evidence (pre-rename, same code): both e2e smokes green on the same 21-study project; old-vs-new Dice at identical 10-round budget: legacy final-global val_acc 0.0063 vs Client-API 0.5217 (local sim: 0.060 vs 0.1875), with best_FL_global_model.pt present in the Client-API results zip.
• Post-rename re-verification on a live stack serving this branch's fl-apps: the renamed spleen tutorial uploaded with "job_type": "standard" went through upload → scan → bundle (new manifest) → train → RESULTS_UPLOADED (model 5de08634), final-global val_acc 0.0132 at the committed 2-round config, with best_FL_global_model.pt in the results zip (the fl-api's submit-time selector injection works through the renamed manifest).

Multi-agent review round (applied in-branch)

A six-pass review (code, tests, comments, silent failures, type design, security) ran against this branch; every confirmed finding is fixed in the six commits after faf2aff2:

• FedOpt stepped on a second fresh-init model copy — stock's generator promotes its own state_dict() as the global and never syncs from the persistor, so round 1 silently replaced the global (any SERVER_CHECKPOINT backbone included) with random init + one step. The generator now syncs its copy from the live global in place before each step (optimizer state preserved via tensor identity), resolves source_model at START_RUN under system_panic (the dispatcher swallows handle_event exceptions), and the recipe gained stock's ensure_config_type_dict normalisation + isolated defaults + a construction-time shape check.
• Silent FULL-sender no-op killed — a round accepting zero client results now aborts the job loudly on BEFORE_AGGREGATION (previously: empty aggregates applied as no-ops to a COMPLETED "trained" model), a wrong weights kind relays an actionable params_type='DIFF' message to the hub, and every failed client task is relayed (pointed fallback naming the trust-side logs when no exception header exists — full traceback transport is Restore end-to-end client exception messages to the hub for Client-API tasks #987).
• Pre-rename *_client_api aliases actually work now — they were unreachable dead code (manifest validation rejected them first); both bundlers normalise them before validation, UnknownJobTypeError names the valid set + fix, and the contract is pinned unmocked against the real manifest.
• Legacy retirement completed — ModelEval, the executors/ package and EvaluationPTModelLocator (all unwired) deleted with their tests; referencing prose reworded historically.
• Template drift guard — new tests pin each committed fl-apps template equal to its recipe's export and assert no deployed aggregator overrides the stock WEIGHT_DIFF default.
• ~25 confirmed doc/comment rot items swept — including the standard README instructing the now-rejected DataKind.WEIGHTS return and nonexistent flare.get(), the .gitattributes linguist-generated globs that matched nothing post-rename, and validator.py lingering in the UI fallback/mock/docs contracts.

The security pass found no vulnerabilities introduced by the PR (the one candidate sink — user-steerable server-side instantiate_class — was traced end-to-end and is not reachable from user input).

Issues found and split out during verification

• XNAT dcm2niix conversion pinned to stale 2021 Docker Hub build silently drops slices from valid series #981 — Container Service dcm2niix pinned to a stale 2021 Docker Hub build silently drops slices from 6/21 spleen studies (draft PR up, CI green).

Ticks the flip-fl-base#116 box on #665 and resolves its "retire the legacy standard Executor job type once parity is confirmed" consideration — parity was confirmed and the legacy syntax retired. The XGBoost port (flip-fl-base#114) remains the last open #665 item.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!