Add per-site client tracking and harden recipe client placement

[YuanTingHsieh]

Summary

• add clients parameter to add_experiment_tracking(...) for client-side receiver targeting; per-site tracking configs (e.g. per-site tracking_uri) are one call per site — replaces the recipe.job.to(receiver, site_name) anti-pattern (Recipe API requirement 14)
• harden the recipe client-placement layer (_add_to_client_apps, used by add_client_config, add_client_file, client filter helpers, and the new tracking targeting): targeting specific clients while the job's client app applies to all clients now raises a clear ValueError instead of the placement being silently dropped at export; targeting a site with no existing client app (when per-site apps exist) raises with the list of configured sites instead of silently deploying a bare, executor-less app to that site; non-list clients (a bare string would iterate per character), empty lists, and non-client names (server, @ALL) are rejected
• docs: add_experiment_tracking signature and per-site usage; a general note that clients= targeting requires per-site client apps (constructor per_site_config), and that set_per_site_config does not yet rebuild an all-clients app (recipes implementing _apply_per_site_config is follow-up work)

Why

Requirement 14: client-side tracking receivers with per-site configs required reaching into recipe.job — the internal the contract page declares off-limits. The placement-layer hardening fixes real pre-existing bugs: targeted add_client_config/add_client_file/filter calls on the default all-clients topology were silently dropped from the exported job.

Tests

• python3 -m pytest tests/unit_test/recipe/ -q (396 passed, 22 torch-gated skips)
• New coverage: per-site tracking configs in-memory and through export; topology/unknown-site/empty-list/non-list/bad-name rejection through the public helpers (add_experiment_tracking, add_client_config, add_client_file)
• black / isort / flake8 clean on changed files

[Copilot]

Pull request overview

This PR extends the NVFlare Recipe API with a bounded, recipe-level component placement surface and enhances experiment tracking to support per-site client receiver targeting without requiring direct recipe.job manipulation. It also updates documentation and adds unit tests to validate placement behavior, topology constraints, and export serialization.

Changes:

• Add Recipe.add_client_component(...) and Recipe.add_server_component(...) as recipe-level primitives for placing plain components into generated client/server app configs.
• Extend add_experiment_tracking(...) with a clients parameter and route client receiver placement through the new Recipe helper to preserve existing per-site client app topologies.
• Update Recipe API documentation and add unit tests covering targeting rules, error guards, and export config serialization.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/unit_test/recipe/utils_test.py	Adds coverage for add_experiment_tracking(..., clients=...) per-site behavior and export survival.
tests/unit_test/recipe/spec_test.py	Adds coverage for new Recipe component placement helpers and their guardrails.
nvflare/recipe/utils.py	Adds clients targeting support to add_experiment_tracking and routes placement via Recipe helper.
nvflare/recipe/spec.py	Implements add_client_component / add_server_component and strengthens client-placement logic.
docs/user_guide/data_scientist_guide/recipe_api.rst	Documents new component helpers and updated tracking signature/semantics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[greptile-apps]

Greptile Summary

This PR adds a clients parameter to add_experiment_tracking for per-site client-side receiver targeting, enabling per-site tracking configurations (e.g. different tracking_uri per site) without reaching into the internal recipe.job API. It also hardens the shared _add_to_client_apps placement layer so that bad placements — targeting specific clients when an all-clients app exists, naming unknown sites in a per-site topology, passing non-list or empty clients values, or using reserved names — all raise descriptive errors instead of silently failing at export time.

• nvflare/recipe/spec.py: _add_to_client_apps now computes the deploy-map state unconditionally and enforces five explicit guards (non-list, empty, reserved-name, all-clients topology mismatch, unknown site in per-site topology) before any placement occurs; all callers (add_client_config, add_client_file, filter helpers, and the new tracking path) inherit these guards automatically.
• nvflare/recipe/utils.py: add_experiment_tracking gains the clients parameter with an upfront validation layer; client-side placement now routes through _add_to_client_apps instead of job.to_clients() to preserve existing per-site topology.
• Tests: 396 pre-existing tests continue to pass; new TestClientPlacementHardening and TestAddExperimentTrackingClients classes cover each new code path, including an export round-trip.

Confidence Score: 5/5

Safe to merge. The placement-layer hardening converts previously silent no-ops and silent drops into explicit errors, which is strictly an improvement over the status quo.

All five new validation guards in _add_to_client_apps are covered by dedicated tests, including the export round-trip. The clients=None path preserves the original per-site fan-out logic unchanged. No circular import risks or state-mutation issues were identified.

No files require special attention. The _add_to_client_apps logic in spec.py is the most complex new code but is fully exercised by the new test suite.

Important Files Changed

Filename	Overview
nvflare/recipe/spec.py	Refactored _add_to_client_apps to compute deploy-map state unconditionally and added comprehensive validation (TypeError for non-list, ValueError for empty/reserved names, and two topology-mismatch guards). Logic is sound; all branches are exercised by the new test suite.
nvflare/recipe/utils.py	Added clients parameter to add_experiment_tracking, with upfront validation for type/empty/non-client-side, then routes through _add_to_client_apps instead of raw job.to_clients() to preserve per-site topology. Previously noted silent no-op for empty list is now fixed.
tests/unit_test/recipe/spec_test.py	New TestClientPlacementHardening class covers all four topology-guard paths (all-clients reject, per-site accept, unknown-site reject) plus type/empty/reserved-name rejections via add_client_config and add_client_file public surfaces.
tests/unit_test/recipe/utils_test.py	New TestAddExperimentTrackingClients covers single-site targeting, per-site multi-call patterns, default all-clients path, server unaffected path, and all rejection cases including the export round-trip test.
docs/user_guide/data_scientist_guide/recipe_api.rst	Updated signature for add_experiment_tracking with the new clients parameter and added a prose block explaining per-site topology requirements, set_per_site_config limitation, and per-site usage pattern. Accurate and aligned with code behaviour.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["add_experiment_tracking(recipe, ..., clients=...)"] --> B{clients is not None?}
    B -- No --> C["_add_to_client_apps(obj, clients=None)"]
    B -- Yes --> D{client_side=True?}
    D -- No --> E["raise ValueError\n(clients only for client_side)"]
    D -- Yes --> F{isinstance list of str?}
    F -- No --> G["raise TypeError"]
    F -- Yes --> H{clients empty?}
    H -- Yes --> I["raise ValueError\n(must not be empty)"]
    H -- No --> J["_add_to_client_apps(obj, clients=clients)"]

    C --> K{existing_client_sites?}
    K -- Yes --> L["job.to(obj, site) for each site"]
    K -- No --> M["job.to_clients(obj)"]

    J --> N{ALL_SITES in deploy_map?}
    N -- Yes --> O["raise ValueError\n(all-clients topology)"]
    N -- No --> P{existing_client_sites non-empty?}
    P -- Yes --> Q{unknown sites?}
    Q -- Yes --> R["raise ValueError\n(list known sites)"]
    Q -- No --> S["job.to(obj, client) for each client"]
    P -- No --> S

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["add_experiment_tracking(recipe, ..., clients=...)"] --> B{clients is not None?}
    B -- No --> C["_add_to_client_apps(obj, clients=None)"]
    B -- Yes --> D{client_side=True?}
    D -- No --> E["raise ValueError\n(clients only for client_side)"]
    D -- Yes --> F{isinstance list of str?}
    F -- No --> G["raise TypeError"]
    F -- Yes --> H{clients empty?}
    H -- Yes --> I["raise ValueError\n(must not be empty)"]
    H -- No --> J["_add_to_client_apps(obj, clients=clients)"]

    C --> K{existing_client_sites?}
    K -- Yes --> L["job.to(obj, site) for each site"]
    K -- No --> M["job.to_clients(obj)"]

    J --> N{ALL_SITES in deploy_map?}
    N -- Yes --> O["raise ValueError\n(all-clients topology)"]
    N -- No --> P{existing_client_sites non-empty?}
    P -- Yes --> Q{unknown sites?}
    Q -- Yes --> R["raise ValueError\n(list known sites)"]
    Q -- No --> S["job.to(obj, client) for each client"]
    P -- No --> S

Loading

_{Reviews (7): Last reviewed commit: "Merge branch 'main' into codex/recipe-co..." | Re-trigger Greptile}

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.32%. Comparing base (94a30d5) to head (28b3f0d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4862      +/-   ##
==========================================
+ Coverage   60.30%   60.32%   +0.02%     
==========================================
  Files         973      973              
  Lines       92233    92253      +20     
==========================================
+ Hits        55619    55655      +36     
+ Misses      36614    36598      -16     

Flag	Coverage Δ
unit-tests	60.32% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.