From d80150da4703056968ded053675362f5817f3aa6 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Mon, 6 Jul 2026 15:23:42 -0700 Subject: [PATCH 1/5] Add recipe-level component placement and per-site tracking - Recipe.add_client_component / Recipe.add_server_component: bounded placement primitive for components (tracking receivers, streamers) so users never need recipe.job.to(...) - add_experiment_tracking gains clients= for client-side targeting; per-site tracking configs are one call per site - guards with guiding errors: str/dict/Filter/Controller/Executor/ script runners are routed to their dedicated APIs - fail loudly when targeting specific clients on the default all-clients topology (previously such placements were silently dropped at export; also fixes the same latent drop in add_client_config/add_client_file/client filters with clients=[...]) - reject non-client names (server, @ALL) in clients lists - contract page updated: component helpers section, tracking signature, revised placement escape hatch Addresses Recipe API requirements 6 and 14; unblocks migrating tensor-stream and job_api tracking examples off recipe.job. Co-Authored-By: Claude Fable 5 --- .../data_scientist_guide/recipe_api.rst | 31 ++- nvflare/recipe/spec.py | 140 ++++++++++- nvflare/recipe/utils.py | 30 ++- tests/unit_test/recipe/spec_test.py | 220 ++++++++++++++++++ tests/unit_test/recipe/utils_test.py | 136 +++++++++++ 5 files changed, 543 insertions(+), 14 deletions(-) diff --git a/docs/user_guide/data_scientist_guide/recipe_api.rst b/docs/user_guide/data_scientist_guide/recipe_api.rst index 652c9b4bb8..5623dca638 100644 --- a/docs/user_guide/data_scientist_guide/recipe_api.rst +++ b/docs/user_guide/data_scientist_guide/recipe_api.rst @@ -98,6 +98,24 @@ File packaging helpers: ``recipe.add_server_file(file_path)`` Bundle a file or directory into the generated server app package. +Component helpers: + +``recipe.add_client_component(component, clients=None, id=None)`` + Add a component object (e.g. a tracking receiver or streamer) to generated + client apps. If ``clients`` is omitted, the component applies to all + generated client apps. + +``recipe.add_server_component(component, id=None)`` + Add a component object to the generated server app. Returns the final + component id. + +These component helpers place plain components only. Files, config parameters, +filters, and executors have the dedicated APIs above; controllers are +configured by the recipe itself. Targeting specific clients with ``clients`` +requires per-site client apps (e.g. recipes configured with per-site config); +with the default all-clients topology, targeted placement raises an error +rather than silently dropping the component from the generated job. + Filter helpers: ``recipe.add_client_input_filter(filter, tasks=None, clients=None)`` @@ -123,9 +141,11 @@ Shared helpers: Utility helpers: -``add_experiment_tracking(recipe, tracking_type, tracking_config=None, client_side=False, server_side=True)`` +``add_experiment_tracking(recipe, tracking_type, tracking_config=None, client_side=False, server_side=True, clients=None)`` Add supported experiment tracking receivers such as TensorBoard, MLflow, or - Weights & Biases. + Weights & Biases. With ``client_side=True``, ``clients`` limits which sites + receive the client-side receiver; call once per site with different + ``tracking_config`` values for per-site tracking destinations. ``add_cross_site_evaluation(recipe, submit_model_timeout=600, validation_timeout=6000, participating_clients=None)`` Add cross-site evaluation to a training recipe when the recipe/framework @@ -228,9 +248,10 @@ The following are internal details and should not be used in recipe scripts: * generated deploy-map internals; * internal fields used by Recipe helpers. -If a workflow needs arbitrary component placement that is not covered by a named -Recipe helper, use the lower-level Job API workflow or add a new named Recipe -helper for the repeated pattern. +Custom component placement is covered by ``add_server_component`` and +``add_client_component``. If a workflow needs job structure those helpers do +not express (e.g. custom executors or multi-app layouts), use the lower-level +Job API workflow or add a new named Recipe helper for the repeated pattern. Recipe Catalog JSON ------------------- diff --git a/nvflare/recipe/spec.py b/nvflare/recipe/spec.py index abd62e7313..114844ba1c 100644 --- a/nvflare/recipe/spec.py +++ b/nvflare/recipe/spec.py @@ -15,7 +15,7 @@ import sys from abc import ABC, abstractmethod from contextlib import contextmanager -from typing import Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Union # Single source of truth for the default export dir. Kept in the current working # directory on purpose: an export is a user-requested artifact, so it lands next to @@ -88,7 +88,9 @@ def _peek_recipe_args() -> tuple: return _RECIPE_EXPORT, _RECIPE_EXPORT_DIR +from nvflare.apis.executor import Executor from nvflare.apis.filter import Filter +from nvflare.apis.impl.controller import Controller from nvflare.app_common.widgets.decomposer_reg import DecomposerRegister from nvflare.fuel.utils.fobs import Decomposer from nvflare.job_config.api import FedJob @@ -314,14 +316,19 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs obj: Object to add to clients. clients: Optional list of specific client names. If None, applies to all clients. **kwargs: Extra options forwarded to `job.to()`/`job.to_clients()`. + + Raises: + ValueError: If clients contains a non-client name, or if specific clients are + targeted while the recipe's client app applies to all clients (per-site + placement cannot be expressed in the generated job in that topology). """ - if clients is None: - from nvflare.apis.job_def import ALL_SITES, SERVER_SITE_NAME - from nvflare.job_config.defs import JobTargetType + from nvflare.apis.job_def import ALL_SITES, SERVER_SITE_NAME + from nvflare.job_config.defs import JobTargetType - # FedJob has no public API to list per-site deploy targets, so we inspect - # private deploy map to preserve existing per-site client topology. - deploy_map = getattr(self.job, "_deploy_map", {}) + # FedJob has no public API to list per-site deploy targets, so we inspect + # private deploy map to preserve existing per-site client topology. + deploy_map = getattr(self.job, "_deploy_map", {}) + if clients is None: existing_client_sites = [ target for target in deploy_map.keys() @@ -334,6 +341,19 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs else: self.job.to_clients(obj, **kwargs) else: + for client in clients: + if not isinstance(client, str) or client in (ALL_SITES, SERVER_SITE_NAME): + raise ValueError(f"invalid client name {client!r}: client names must name specific client sites") + if ALL_SITES in deploy_map: + # The generated job has one client app deployed to all clients. Exporting + # both an all-clients app and per-site apps is not expressible (the + # all-clients app wins and per-site apps are dropped), so fail loudly + # instead of silently losing the placement. + raise ValueError( + "cannot target specific clients: this recipe's client app applies to all clients. " + "Configure per-site client apps first (e.g. per_site_config/set_per_site_config) " + "or omit clients to apply to all clients." + ) for client in clients: self.job.to(obj, client, **kwargs) @@ -407,6 +427,75 @@ def add_client_file(self, file_path: str, clients: Optional[List[str]] = None): self._add_to_client_apps(file_path, clients=clients) + def add_client_component(self, component: Any, clients: Optional[List[str]] = None, id: Optional[str] = None): + """Add a component to client apps. + + Use this for client-side components such as tracking receivers or streamers, + instead of reaching into the recipe's generated job. The component is + registered in each targeted client app's configuration. + + This is a bounded placement primitive for components only. Files, config + parameters, filters, and executors have dedicated APIs: ``add_client_file``, + ``add_client_config``, ``add_client_input_filter``/``add_client_output_filter``, + and the recipe's own executor/train-script parameters. + + Args: + component: The component object to add to client apps. + clients: Optional list of specific client names. If None, applies to all clients. + Targeting specific clients requires the recipe's client apps to be per-site + (e.g. recipes configured with per-site config); with the default all-clients + topology, targeted placement raises ValueError instead of silently dropping + the component from the generated job. + id: Optional component id. If None, an id is generated. If the id is already + used in a client app, a numeric suffix is appended. + + Raises: + TypeError: If component is a str, dict, Filter, Controller, Executor, or a + script runner. + ValueError: If clients names non-client sites, or targets specific clients + while the recipe's client app applies to all clients. + + Example: + # Add a streamer component to all clients + recipe.add_client_component(TensorClientStreamer()) + + # Add a tracking receiver to one site only (per-site client apps required) + recipe.add_client_component(receiver, clients=["site-1"], id="mlflow_receiver") + """ + self._validate_placed_component(component, "client") + if isinstance(component, Controller): + raise TypeError( + "Controllers run in the server workflow and are configured by the recipe; " + "they cannot be added to clients" + ) + self._add_to_client_apps(component, clients=clients, id=id) + + @staticmethod + def _validate_placed_component(component: Any, target_type: str) -> None: + """Reject objects that have dedicated APIs so component placement stays bounded.""" + if isinstance(component, str): + raise TypeError(f"component must be an object; use add_{target_type}_file for files or scripts") + if isinstance(component, dict): + raise TypeError(f"component must be an object; use add_{target_type}_config for configuration parameters") + if isinstance(component, Filter): + raise TypeError( + f"Filters are wired into filter chains; use add_{target_type}_input_filter or " + f"add_{target_type}_output_filter" + ) + if isinstance(component, Executor): + raise TypeError( + "Executors are configured through the recipe's executor/train-script parameters, " + f"not add_{target_type}_component" + ) + # Script runners are executor-installing wrappers, not plain components. + from nvflare.job_config.script_runner import BaseScriptRunner, ScriptRunner + + if isinstance(component, (BaseScriptRunner, ScriptRunner)): + raise TypeError( + "script runners are configured through the recipe's train-script parameters, " + f"not add_{target_type}_component" + ) + def add_server_output_filter(self, filter: Filter, tasks: Optional[List[str]] = None): """Add a filter to the server for outgoing tasks to clients. @@ -466,6 +555,43 @@ def add_server_file(self, file_path: str): self.job.to_server(file_path) + def add_server_component(self, component: Any, id: Optional[str] = None) -> Any: + """Add a component to the server app. + + Use this for server-side components such as tracking receivers or streamers, + instead of reaching into the recipe's generated job. The component is + registered in the server app's configuration. + + This is a bounded placement primitive for components only. Files, config + parameters, and filters have dedicated APIs: ``add_server_file``, + ``add_server_config``, and ``add_server_input_filter``/``add_server_output_filter``. + Controllers are configured by the recipe itself. + + Args: + component: The component object to add to the server app. + id: Optional component id. If None, an id is generated. If the id is already + used in the server app, a numeric suffix is appended. + + Returns: + The final component id assigned in the server app. Components that implement + their own job-registration hook (``add_to_fed_job``) return that hook's result + instead, which may not be an id. + + Raises: + TypeError: If component is a str, dict, Filter, Controller, Executor, or a + script runner. + + Example: + # Add a streamer component to the server + streamer_id = recipe.add_server_component(TensorServerStreamer()) + """ + self._validate_placed_component(component, "server") + if isinstance(component, Controller): + raise TypeError( + "Controllers are configured by the recipe itself; they cannot be added through " "add_server_component" + ) + return self.job.to_server(component, id=id) + @staticmethod def _get_full_class_name(obj): """ diff --git a/nvflare/recipe/utils.py b/nvflare/recipe/utils.py index 8eaeff97d8..be4c7b08d8 100644 --- a/nvflare/recipe/utils.py +++ b/nvflare/recipe/utils.py @@ -214,6 +214,7 @@ def add_experiment_tracking( tracking_config: Optional[dict] = None, client_side: bool = False, server_side: bool = True, + clients: Optional[List[str]] = None, ): """Add experiment tracking to a recipe. @@ -223,8 +224,15 @@ def add_experiment_tracking( recipe: Recipe instance to augment with experiment tracking. tracking_type: Type of tracking to enable ("mlflow", "tensorboard", or "wandb"). tracking_config: Optional configuration dict for the tracking receiver. - client_side: If True, add tracking to all clients (each client tracks locally). + client_side: If True, add tracking to clients (each client tracks locally). server_side: If True, add tracking to server (aggregates metrics from all clients). Default: True. + clients: Optional list of client names for client-side tracking. If None, the + client-side receiver is added to all clients. Only valid with client_side=True. + To give sites different receiver configs (e.g. per-site tracking_uri), call this + function once per site with that site's tracking_config and clients=[site]. + Targeting specific clients requires the recipe's client apps to be per-site + (e.g. recipes configured with per-site config); with the default all-clients + topology, targeted placement raises ValueError. Examples: # Server-side tracking (default - federated metrics) @@ -235,6 +243,16 @@ def add_experiment_tracking( # Both server and client tracking add_experiment_tracking(recipe, "mlflow", {...}, client_side=True, server_side=True) + + # Per-site client tracking configs (one call per site) + add_experiment_tracking( + recipe, "mlflow", {"tracking_uri": "file:///tmp/site-1/mlruns"}, + client_side=True, server_side=False, clients=["site-1"], + ) + add_experiment_tracking( + recipe, "mlflow", {"tracking_uri": "file:///tmp/site-2/mlruns"}, + client_side=True, server_side=False, clients=["site-2"], + ) """ tracking_config = tracking_config or {} if tracking_type not in TRACKING_REGISTRY: @@ -243,6 +261,12 @@ def add_experiment_tracking( if not server_side and not client_side: raise ValueError("At least one of server_side or client_side must be True") + if clients is not None: + if not client_side: + raise ValueError("clients is only used for client-side tracking; set client_side=True") + if not isinstance(clients, list) or not all(isinstance(c, str) for c in clients): + raise TypeError(f"clients must be a list of str, got {clients!r}") + _, flag = optional_import(TRACKING_REGISTRY[tracking_type]["package"]) if not flag: raise ValueError( @@ -267,7 +291,9 @@ def add_experiment_tracking( client_config["events"] = [ANALYTIC_EVENT_TYPE] client_receiver = receiver_class(**client_config) - recipe.job.to_clients(client_receiver, id="client_receiver") + # Route through the recipe placement helper so existing per-site client apps + # are preserved (to_clients would target ALL_SITES even when per-site apps exist). + recipe.add_client_component(client_receiver, clients=clients, id="client_receiver") def add_cross_site_evaluation( diff --git a/tests/unit_test/recipe/spec_test.py b/tests/unit_test/recipe/spec_test.py index d14f1ed52d..8dcbf84996 100644 --- a/tests/unit_test/recipe/spec_test.py +++ b/tests/unit_test/recipe/spec_test.py @@ -756,3 +756,223 @@ def __init__(self): with pytest.raises(TypeError, match=match): set_per_site_config(BasicRecipe(), config) + + +class _PlainComponent: + """Minimal component for placement tests.""" + + def __init__(self, tag: str = ""): + self.tag = tag + + +class TestRecipeComponentPlacement: + """Test add_server_component and add_client_component methods in Recipe.""" + + def _make_recipe(self, name="test_component_placement"): + from nvflare.recipe.spec import Recipe + + return Recipe(FedJob(name=name, min_clients=1)) + + def test_add_server_component_registers_and_returns_id(self): + recipe = self._make_recipe() + comp = _PlainComponent() + + cid = recipe.add_server_component(comp, id="my_comp") + + assert cid == "my_comp" + server_app = recipe.job._deploy_map.get("server") + assert server_app is not None + assert server_app.app_config.components[cid] is comp + + def test_add_server_component_generates_id_when_none(self): + recipe = self._make_recipe() + comp = _PlainComponent() + + cid = recipe.add_server_component(comp) + + assert cid + assert recipe.job._deploy_map["server"].app_config.components[cid] is comp + + def test_add_server_component_duplicate_id_gets_suffix(self): + recipe = self._make_recipe() + first = _PlainComponent("first") + second = _PlainComponent("second") + + cid1 = recipe.add_server_component(first, id="recv") + cid2 = recipe.add_server_component(second, id="recv") + + assert cid1 == "recv" + assert cid2 != cid1 + components = recipe.job._deploy_map["server"].app_config.components + assert components[cid1] is first + assert components[cid2] is second + + def test_add_client_component_all_clients(self): + from nvflare.apis.job_def import ALL_SITES + + recipe = self._make_recipe() + comp = _PlainComponent() + + recipe.add_client_component(comp, id="recv") + + all_clients_app = recipe.job._deploy_map.get(ALL_SITES) + assert all_clients_app is not None + assert all_clients_app.app_config.components["recv"] is comp + + def test_add_client_component_specific_clients(self): + from nvflare.apis.job_def import ALL_SITES + + recipe = self._make_recipe() + comp = _PlainComponent() + + recipe.add_client_component(comp, clients=["site-1"], id="recv") + + assert recipe.job._deploy_map["site-1"].app_config.components["recv"] is comp + assert ALL_SITES not in recipe.job._deploy_map + assert "site-2" not in recipe.job._deploy_map + + def test_add_client_component_preserves_per_site_apps(self): + from nvflare.apis.job_def import ALL_SITES + + recipe = self._make_recipe() + # Existing per-site client topology. + recipe.job.to(_PlainComponent("site-1-only"), "site-1") + recipe.job.to(_PlainComponent("site-2-only"), "site-2") + + shared = _PlainComponent("shared") + recipe.add_client_component(shared, id="recv") + + # Added to every existing per-site app instead of creating an ALL_SITES app. + assert ALL_SITES not in recipe.job._deploy_map + for site in ("site-1", "site-2"): + components = recipe.job._deploy_map[site].app_config.components + assert any(c is shared for c in components.values()) + + @pytest.mark.parametrize( + "component, match", + [ + ("component.py", "use add_client_file"), + ({"param": 1}, "use add_client_config"), + ], + ) + def test_add_client_component_rejects_str_and_dict(self, component, match): + recipe = self._make_recipe() + with pytest.raises(TypeError, match=match): + recipe.add_client_component(component) + + @pytest.mark.parametrize( + "component, match", + [ + ("component.py", "use add_server_file"), + ({"param": 1}, "use add_server_config"), + ], + ) + def test_add_server_component_rejects_str_and_dict(self, component, match): + recipe = self._make_recipe() + with pytest.raises(TypeError, match=match): + recipe.add_server_component(component) + + def test_add_client_component_rejects_controller(self): + from nvflare.apis.impl.controller import Controller + + class _DummyController(Controller): + def control_flow(self, abort_signal, fl_ctx): + pass + + def start_controller(self, fl_ctx): + pass + + def stop_controller(self, fl_ctx): + pass + + recipe = self._make_recipe() + with pytest.raises(TypeError, match="Controllers"): + recipe.add_client_component(_DummyController()) + + @pytest.mark.parametrize("method", ["add_client_component", "add_server_component"]) + def test_component_methods_reject_executor(self, method): + from nvflare.apis.executor import Executor + + class _DummyExecutor(Executor): + def execute(self, task_name, shareable, fl_ctx, abort_signal): + return None + + recipe = self._make_recipe() + with pytest.raises(TypeError, match="Executors"): + getattr(recipe, method)(_DummyExecutor()) + + def test_add_client_component_specific_clients_rejects_all_sites_topology(self): + recipe = self._make_recipe() + # Default recipe topology: one client app for all clients. + recipe.job.to_clients(_PlainComponent("executor-standin")) + + with pytest.raises(ValueError, match="applies to all clients"): + recipe.add_client_component(_PlainComponent(), clients=["site-1"]) + + @pytest.mark.parametrize("bad_site", ["server", "@ALL"]) + def test_add_client_component_rejects_non_client_site_names(self, bad_site): + recipe = self._make_recipe() + with pytest.raises(ValueError, match="invalid client name"): + recipe.add_client_component(_PlainComponent(), clients=[bad_site]) + + @pytest.mark.parametrize("method", ["add_client_component", "add_server_component"]) + def test_component_methods_reject_filter(self, method): + from nvflare.apis.filter import Filter + + class _DummyFilter(Filter): + def process(self, shareable, fl_ctx): + return shareable + + recipe = self._make_recipe() + with pytest.raises(TypeError, match="input_filter"): + getattr(recipe, method)(_DummyFilter()) + + def test_add_server_component_rejects_controller(self): + from nvflare.apis.impl.controller import Controller + + class _DummyController(Controller): + def control_flow(self, abort_signal, fl_ctx): + pass + + def start_controller(self, fl_ctx): + pass + + def stop_controller(self, fl_ctx): + pass + + recipe = self._make_recipe() + with pytest.raises(TypeError, match="Controllers"): + recipe.add_server_component(_DummyController()) + + @pytest.mark.parametrize("method", ["add_client_component", "add_server_component"]) + def test_component_methods_reject_script_runner(self, method): + from nvflare.job_config.script_runner import FrameworkType, ScriptRunner + + runner = ScriptRunner(script="train.py", framework=FrameworkType.RAW) + recipe = self._make_recipe() + with pytest.raises(TypeError, match="script runners"): + getattr(recipe, method)(runner) + + def test_components_appear_in_exported_job_config(self, tmp_path): + recipe = self._make_recipe("test_component_export") + recipe.job.to_server({"server_arg": True}) + server_comp = _PlainComponent("server-side") + client_comp = _PlainComponent("client-side") + + recipe.add_server_component(server_comp, id="server_recv") + recipe.add_client_component(client_comp, id="client_recv") + + recipe.job.export_job(str(tmp_path)) + job_dir = tmp_path / "test_component_export" + + with open(job_dir / "app" / "config" / "config_fed_server.json") as f: + server_cfg = json.load(f) + with open(job_dir / "app" / "config" / "config_fed_client.json") as f: + client_cfg = json.load(f) + + server_entry = next(c for c in server_cfg["components"] if c["id"] == "server_recv") + assert server_entry["path"].endswith("_PlainComponent") + assert server_entry["args"] == {"tag": "server-side"} + client_entry = next(c for c in client_cfg["components"] if c["id"] == "client_recv") + assert client_entry["path"].endswith("_PlainComponent") + assert client_entry["args"] == {"tag": "client-side"} diff --git a/tests/unit_test/recipe/utils_test.py b/tests/unit_test/recipe/utils_test.py index 883a29b89a..95682b72b4 100644 --- a/tests/unit_test/recipe/utils_test.py +++ b/tests/unit_test/recipe/utils_test.py @@ -523,3 +523,139 @@ def __init__(self, *args, **kwargs): add_cross_site_evaluation(recipe, participating_clients=participating_clients) assert captured_kwargs["participating_clients"] == participating_clients + + +class TestAddExperimentTrackingClients: + """Test client targeting and per-site configs in add_experiment_tracking.""" + + @pytest.fixture + def dummy_tracking(self, monkeypatch): + """Register a dependency-free tracking type backed by types.SimpleNamespace.""" + import nvflare.recipe.utils as utils_mod + + monkeypatch.setitem( + utils_mod.TRACKING_REGISTRY, + "dummy", + # json is always importable; argparse.Namespace(**config) acts as the receiver + # (unlike types.SimpleNamespace it exposes __module__, so it also survives export). + {"package": "json", "receiver_module": "argparse", "receiver_class": "Namespace"}, + ) + return "dummy" + + def _make_recipe(self, name="test_tracking_clients"): + return Recipe(FedJob(name=name, min_clients=1)) + + def test_client_side_tracking_specific_clients(self, dummy_tracking): + from nvflare.apis.analytix import ANALYTIC_EVENT_TYPE + from nvflare.apis.job_def import ALL_SITES + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + add_experiment_tracking( + recipe, + dummy_tracking, + {"tracking_uri": "file:///tmp/site-1/mlruns"}, + client_side=True, + server_side=False, + clients=["site-1"], + ) + + receiver = recipe.job._deploy_map["site-1"].app_config.components["client_receiver"] + assert receiver.tracking_uri == "file:///tmp/site-1/mlruns" + # Local (non-federated) analytics events are configured by default. + assert receiver.events == [ANALYTIC_EVENT_TYPE] + assert ALL_SITES not in recipe.job._deploy_map + + def test_client_side_tracking_per_site_configs(self, dummy_tracking): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + for site in ("site-1", "site-2"): + add_experiment_tracking( + recipe, + dummy_tracking, + {"tracking_uri": f"file:///tmp/{site}/mlruns"}, + client_side=True, + server_side=False, + clients=[site], + ) + + for site in ("site-1", "site-2"): + receiver = recipe.job._deploy_map[site].app_config.components["client_receiver"] + assert receiver.tracking_uri == f"file:///tmp/{site}/mlruns" + + def test_client_side_tracking_all_clients_by_default(self, dummy_tracking): + from nvflare.apis.job_def import ALL_SITES + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + add_experiment_tracking(recipe, dummy_tracking, {"tracking_uri": "u"}, client_side=True, server_side=False) + + receiver = recipe.job._deploy_map[ALL_SITES].app_config.components["client_receiver"] + assert receiver.tracking_uri == "u" + + def test_server_side_tracking_unaffected_by_clients_feature(self, dummy_tracking): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + add_experiment_tracking(recipe, dummy_tracking, {"tracking_uri": "u"}) + + components = recipe.job._deploy_map["server"].app_config.components + assert components["receiver"].tracking_uri == "u" + # Server receiver keeps federated (default) events. + assert not hasattr(components["receiver"], "events") + + def test_clients_without_client_side_raises(self, dummy_tracking): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + with pytest.raises(ValueError, match="client_side=True"): + add_experiment_tracking(recipe, dummy_tracking, {"tracking_uri": "u"}, clients=["site-1"]) + + @pytest.mark.parametrize("bad_clients", ["site-1", [1, 2], [None]]) + def test_clients_must_be_list_of_str(self, dummy_tracking, bad_clients): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + with pytest.raises(TypeError, match="clients must be a list of str"): + add_experiment_tracking( + recipe, dummy_tracking, {"tracking_uri": "u"}, client_side=True, clients=bad_clients + ) + + def test_clients_targeting_rejects_all_sites_topology(self, dummy_tracking): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + # Default recipe topology: one client app for all clients. + recipe.job.to_clients({"executor_standin": True}) + + with pytest.raises(ValueError, match="applies to all clients"): + add_experiment_tracking( + recipe, + dummy_tracking, + {"tracking_uri": "u"}, + client_side=True, + server_side=False, + clients=["site-1"], + ) + + def test_per_site_client_receiver_survives_export(self, dummy_tracking, tmp_path): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe("test_tracking_export") + recipe.job.to_server({"server_arg": True}) + add_experiment_tracking( + recipe, + dummy_tracking, + {"tracking_uri": "file:///tmp/site-1/mlruns"}, + client_side=True, + server_side=False, + clients=["site-1"], + ) + + recipe.job.export_job(str(tmp_path)) + with open(tmp_path / "test_tracking_export" / "app_site-1" / "config" / "config_fed_client.json") as f: + client_cfg = json.load(f) + + entry = next(c for c in client_cfg["components"] if c["id"] == "client_receiver") + assert entry["path"].endswith("Namespace") From 61c76425b1ccdb9b8efb30fb4e585440f1f76459 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Mon, 6 Jul 2026 17:54:25 -0700 Subject: [PATCH 2/5] Address component placement review feedback - reject non-list clients (a bare string would iterate per character) and empty clients lists (previously a silent no-op) in _add_to_client_apps and add_experiment_tracking - consolidate the Controller guard into _validate_placed_component, checked before Executor so a hybrid subclass gets the accurate message; removes the duplicated per-method checks - document that add_client_component returns None (per-app id suffixing means no single id exists) and reflect the add_to_fed_job return caveat on the contract page - collapse implicit string-literal concatenation in guard messages into single literals Co-Authored-By: Claude Fable 5 --- .../data_scientist_guide/recipe_api.rst | 3 +- nvflare/recipe/spec.py | 50 +++++++++++-------- nvflare/recipe/utils.py | 2 + tests/unit_test/recipe/spec_test.py | 11 ++++ tests/unit_test/recipe/utils_test.py | 7 +++ 5 files changed, 51 insertions(+), 22 deletions(-) diff --git a/docs/user_guide/data_scientist_guide/recipe_api.rst b/docs/user_guide/data_scientist_guide/recipe_api.rst index 5623dca638..7b49c81764 100644 --- a/docs/user_guide/data_scientist_guide/recipe_api.rst +++ b/docs/user_guide/data_scientist_guide/recipe_api.rst @@ -107,7 +107,8 @@ Component helpers: ``recipe.add_server_component(component, id=None)`` Add a component object to the generated server app. Returns the final - component id. + component id; components that implement their own job-registration hook + return that hook's result instead. These component helpers place plain components only. Files, config parameters, filters, and executors have the dedicated APIs above; controllers are diff --git a/nvflare/recipe/spec.py b/nvflare/recipe/spec.py index 114844ba1c..413ff4694c 100644 --- a/nvflare/recipe/spec.py +++ b/nvflare/recipe/spec.py @@ -318,9 +318,10 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs **kwargs: Extra options forwarded to `job.to()`/`job.to_clients()`. Raises: - ValueError: If clients contains a non-client name, or if specific clients are - targeted while the recipe's client app applies to all clients (per-site - placement cannot be expressed in the generated job in that topology). + TypeError: If clients is not a list. + ValueError: If clients is empty or contains a non-client name, or if specific + clients are targeted while the recipe's client app applies to all clients + (per-site placement cannot be expressed in the generated job in that topology). """ from nvflare.apis.job_def import ALL_SITES, SERVER_SITE_NAME from nvflare.job_config.defs import JobTargetType @@ -341,6 +342,11 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs else: self.job.to_clients(obj, **kwargs) else: + # A bare string would iterate per character; an empty list would be a silent no-op. + if not isinstance(clients, list): + raise TypeError(f"clients must be a list of client names, got {type(clients).__name__}") + if not clients: + raise ValueError("clients must not be empty; omit it to apply to all clients") for client in clients: if not isinstance(client, str) or client in (ALL_SITES, SERVER_SITE_NAME): raise ValueError(f"invalid client name {client!r}: client names must name specific client sites") @@ -449,11 +455,16 @@ def add_client_component(self, component: Any, clients: Optional[List[str]] = No id: Optional component id. If None, an id is generated. If the id is already used in a client app, a numeric suffix is appended. + Returns: + None. The final id may differ per client app (numeric suffixing is per app), + so no single id is returned; pass an explicit id that is unused in the + targeted apps if you need a stable reference. + Raises: TypeError: If component is a str, dict, Filter, Controller, Executor, or a - script runner. - ValueError: If clients names non-client sites, or targets specific clients - while the recipe's client app applies to all clients. + script runner, or clients is not a list. + ValueError: If clients is empty or names non-client sites, or targets specific + clients while the recipe's client app applies to all clients. Example: # Add a streamer component to all clients @@ -463,11 +474,6 @@ def add_client_component(self, component: Any, clients: Optional[List[str]] = No recipe.add_client_component(receiver, clients=["site-1"], id="mlflow_receiver") """ self._validate_placed_component(component, "client") - if isinstance(component, Controller): - raise TypeError( - "Controllers run in the server workflow and are configured by the recipe; " - "they cannot be added to clients" - ) self._add_to_client_apps(component, clients=clients, id=id) @staticmethod @@ -479,21 +485,27 @@ def _validate_placed_component(component: Any, target_type: str) -> None: raise TypeError(f"component must be an object; use add_{target_type}_config for configuration parameters") if isinstance(component, Filter): raise TypeError( - f"Filters are wired into filter chains; use add_{target_type}_input_filter or " - f"add_{target_type}_output_filter" + f"Filters are wired into filter chains; use add_{target_type}_input_filter or add_{target_type}_output_filter" + ) + # Check Controller before Executor so a hybrid subclass gets the accurate message. + if isinstance(component, Controller): + if target_type == "client": + raise TypeError( + "Controllers run in the server workflow and are configured by the recipe; they cannot be added to clients" + ) + raise TypeError( + "Controllers are configured by the recipe itself; they cannot be added through add_server_component" ) if isinstance(component, Executor): raise TypeError( - "Executors are configured through the recipe's executor/train-script parameters, " - f"not add_{target_type}_component" + f"Executors are configured through the recipe's executor/train-script parameters, not add_{target_type}_component" ) # Script runners are executor-installing wrappers, not plain components. from nvflare.job_config.script_runner import BaseScriptRunner, ScriptRunner if isinstance(component, (BaseScriptRunner, ScriptRunner)): raise TypeError( - "script runners are configured through the recipe's train-script parameters, " - f"not add_{target_type}_component" + f"script runners are configured through the recipe's train-script parameters, not add_{target_type}_component" ) def add_server_output_filter(self, filter: Filter, tasks: Optional[List[str]] = None): @@ -586,10 +598,6 @@ def add_server_component(self, component: Any, id: Optional[str] = None) -> Any: streamer_id = recipe.add_server_component(TensorServerStreamer()) """ self._validate_placed_component(component, "server") - if isinstance(component, Controller): - raise TypeError( - "Controllers are configured by the recipe itself; they cannot be added through " "add_server_component" - ) return self.job.to_server(component, id=id) @staticmethod diff --git a/nvflare/recipe/utils.py b/nvflare/recipe/utils.py index be4c7b08d8..c9e3067292 100644 --- a/nvflare/recipe/utils.py +++ b/nvflare/recipe/utils.py @@ -266,6 +266,8 @@ def add_experiment_tracking( raise ValueError("clients is only used for client-side tracking; set client_side=True") if not isinstance(clients, list) or not all(isinstance(c, str) for c in clients): raise TypeError(f"clients must be a list of str, got {clients!r}") + if not clients: + raise ValueError("clients must not be empty; omit it to add tracking to all clients") _, flag = optional_import(TRACKING_REGISTRY[tracking_type]["package"]) if not flag: diff --git a/tests/unit_test/recipe/spec_test.py b/tests/unit_test/recipe/spec_test.py index 8dcbf84996..947d5d0bcd 100644 --- a/tests/unit_test/recipe/spec_test.py +++ b/tests/unit_test/recipe/spec_test.py @@ -915,6 +915,17 @@ def test_add_client_component_rejects_non_client_site_names(self, bad_site): with pytest.raises(ValueError, match="invalid client name"): recipe.add_client_component(_PlainComponent(), clients=[bad_site]) + def test_add_client_component_rejects_bare_string_clients(self): + recipe = self._make_recipe() + # A bare string would otherwise iterate per character and create per-char apps. + with pytest.raises(TypeError, match="must be a list"): + recipe.add_client_component(_PlainComponent(), clients="site-1") + + def test_add_client_component_rejects_empty_clients(self): + recipe = self._make_recipe() + with pytest.raises(ValueError, match="must not be empty"): + recipe.add_client_component(_PlainComponent(), clients=[]) + @pytest.mark.parametrize("method", ["add_client_component", "add_server_component"]) def test_component_methods_reject_filter(self, method): from nvflare.apis.filter import Filter diff --git a/tests/unit_test/recipe/utils_test.py b/tests/unit_test/recipe/utils_test.py index 95682b72b4..6391a58fde 100644 --- a/tests/unit_test/recipe/utils_test.py +++ b/tests/unit_test/recipe/utils_test.py @@ -622,6 +622,13 @@ def test_clients_must_be_list_of_str(self, dummy_tracking, bad_clients): recipe, dummy_tracking, {"tracking_uri": "u"}, client_side=True, clients=bad_clients ) + def test_clients_empty_list_raises(self, dummy_tracking): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + with pytest.raises(ValueError, match="must not be empty"): + add_experiment_tracking(recipe, dummy_tracking, {"tracking_uri": "u"}, client_side=True, clients=[]) + def test_clients_targeting_rejects_all_sites_topology(self, dummy_tracking): from nvflare.recipe.utils import add_experiment_tracking From 4e3184735d53b376da874f73d315e0cd9e1a94d6 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Tue, 7 Jul 2026 14:45:17 -0700 Subject: [PATCH 3/5] Recommend constructor per_site_config for targeted placement set_per_site_config currently only records helper config for configured_sites(); the base _apply_per_site_config hook is a no-op and no concrete recipe overrides it yet, so it does not rebuild an existing all-clients app into per-site apps. Pointing the targeted-placement error at it was misleading: following that advice would not resolve the error. Error message, docstrings, and the contract page now recommend the per_site_config constructor argument, and the contract page states explicitly that helper-driven per-site topology is follow-up work per recipe. Co-Authored-By: Claude Fable 5 --- docs/user_guide/data_scientist_guide/recipe_api.rst | 11 ++++++++--- nvflare/recipe/spec.py | 10 +++++----- nvflare/recipe/utils.py | 4 ++-- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/docs/user_guide/data_scientist_guide/recipe_api.rst b/docs/user_guide/data_scientist_guide/recipe_api.rst index 7b49c81764..57b335af7f 100644 --- a/docs/user_guide/data_scientist_guide/recipe_api.rst +++ b/docs/user_guide/data_scientist_guide/recipe_api.rst @@ -113,9 +113,14 @@ Component helpers: These component helpers place plain components only. Files, config parameters, filters, and executors have the dedicated APIs above; controllers are configured by the recipe itself. Targeting specific clients with ``clients`` -requires per-site client apps (e.g. recipes configured with per-site config); -with the default all-clients topology, targeted placement raises an error -rather than silently dropping the component from the generated job. +requires per-site client apps: construct the recipe with the +``per_site_config`` constructor argument on recipes that support it. With the +default all-clients topology, targeted placement raises an error rather than +silently dropping the component from the generated job. Calling +``set_per_site_config`` after construction records the configuration for +``configured_sites()`` but does not yet rebuild an existing all-clients app +into per-site apps; recipes will interpret helper-provided per-site config as +follow-up work. Filter helpers: diff --git a/nvflare/recipe/spec.py b/nvflare/recipe/spec.py index 413ff4694c..10819b17b8 100644 --- a/nvflare/recipe/spec.py +++ b/nvflare/recipe/spec.py @@ -357,8 +357,8 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs # instead of silently losing the placement. raise ValueError( "cannot target specific clients: this recipe's client app applies to all clients. " - "Configure per-site client apps first (e.g. per_site_config/set_per_site_config) " - "or omit clients to apply to all clients." + "Construct the recipe with per-site client apps (e.g. the per_site_config constructor " + "argument on recipes that support it) or omit clients to apply to all clients." ) for client in clients: self.job.to(obj, client, **kwargs) @@ -449,9 +449,9 @@ def add_client_component(self, component: Any, clients: Optional[List[str]] = No component: The component object to add to client apps. clients: Optional list of specific client names. If None, applies to all clients. Targeting specific clients requires the recipe's client apps to be per-site - (e.g. recipes configured with per-site config); with the default all-clients - topology, targeted placement raises ValueError instead of silently dropping - the component from the generated job. + (e.g. recipes constructed with the per_site_config constructor argument); + with the default all-clients topology, targeted placement raises ValueError + instead of silently dropping the component from the generated job. id: Optional component id. If None, an id is generated. If the id is already used in a client app, a numeric suffix is appended. diff --git a/nvflare/recipe/utils.py b/nvflare/recipe/utils.py index c9e3067292..86a70bc70a 100644 --- a/nvflare/recipe/utils.py +++ b/nvflare/recipe/utils.py @@ -231,8 +231,8 @@ def add_experiment_tracking( To give sites different receiver configs (e.g. per-site tracking_uri), call this function once per site with that site's tracking_config and clients=[site]. Targeting specific clients requires the recipe's client apps to be per-site - (e.g. recipes configured with per-site config); with the default all-clients - topology, targeted placement raises ValueError. + (e.g. recipes constructed with the per_site_config constructor argument); + with the default all-clients topology, targeted placement raises ValueError. Examples: # Server-side tracking (default - federated metrics) From d2bee546f2d56347e13db61bd23aa49b4ad3c732 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 15:05:21 -0700 Subject: [PATCH 4/5] Withdraw generic component placement methods; keep tracking + hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A requirement trace showed no current requirement needs public add_client_component/add_server_component: Req 14 (per-site tracking receivers) is fully covered by add_experiment_tracking(clients=...); Req 6 (deploy map) is per-site app topology, not component placement; and an inventory of all recipe.job.to* usages in the tree maps every real placement to an existing named helper (add_cross_site_evaluation), a dedicated API (add_server_config), or a design-doc-classified lower-level Job API example (tensor-stream). Per the 2.9 design decision, arbitrary component placement stays out of the public Recipe contract; this also resolves the reviewer's add_to_fed_job hook contract question by removing the surface it applied to. Kept: the _add_to_client_apps hardening (silent-drop topology guard, non-list/empty/bad-name rejection) — it protects the existing public clients=-taking helpers and the tracking targeting path — with test coverage ported to those public surfaces. --- .../data_scientist_guide/recipe_api.rst | 27 +- nvflare/recipe/spec.py | 112 +-------- nvflare/recipe/utils.py | 4 +- tests/unit_test/recipe/spec_test.py | 237 +++--------------- 4 files changed, 41 insertions(+), 339 deletions(-) diff --git a/docs/user_guide/data_scientist_guide/recipe_api.rst b/docs/user_guide/data_scientist_guide/recipe_api.rst index 57b335af7f..f0788e18c7 100644 --- a/docs/user_guide/data_scientist_guide/recipe_api.rst +++ b/docs/user_guide/data_scientist_guide/recipe_api.rst @@ -98,25 +98,11 @@ File packaging helpers: ``recipe.add_server_file(file_path)`` Bundle a file or directory into the generated server app package. -Component helpers: - -``recipe.add_client_component(component, clients=None, id=None)`` - Add a component object (e.g. a tracking receiver or streamer) to generated - client apps. If ``clients`` is omitted, the component applies to all - generated client apps. - -``recipe.add_server_component(component, id=None)`` - Add a component object to the generated server app. Returns the final - component id; components that implement their own job-registration hook - return that hook's result instead. - -These component helpers place plain components only. Files, config parameters, -filters, and executors have the dedicated APIs above; controllers are -configured by the recipe itself. Targeting specific clients with ``clients`` +Helpers that accept ``clients`` target specific generated client apps. This requires per-site client apps: construct the recipe with the ``per_site_config`` constructor argument on recipes that support it. With the -default all-clients topology, targeted placement raises an error rather than -silently dropping the component from the generated job. Calling +default all-clients topology, targeted calls raise an error rather than +silently dropping the change from the generated job. Calling ``set_per_site_config`` after construction records the configuration for ``configured_sites()`` but does not yet rebuild an existing all-clients app into per-site apps; recipes will interpret helper-provided per-site config as @@ -254,10 +240,9 @@ The following are internal details and should not be used in recipe scripts: * generated deploy-map internals; * internal fields used by Recipe helpers. -Custom component placement is covered by ``add_server_component`` and -``add_client_component``. If a workflow needs job structure those helpers do -not express (e.g. custom executors or multi-app layouts), use the lower-level -Job API workflow or add a new named Recipe helper for the repeated pattern. +If a workflow needs arbitrary component placement that is not covered by a named +Recipe helper, use the lower-level Job API workflow or add a new named Recipe +helper for the repeated pattern. Recipe Catalog JSON ------------------- diff --git a/nvflare/recipe/spec.py b/nvflare/recipe/spec.py index 10819b17b8..9e54c4c4ab 100644 --- a/nvflare/recipe/spec.py +++ b/nvflare/recipe/spec.py @@ -15,7 +15,7 @@ import sys from abc import ABC, abstractmethod from contextlib import contextmanager -from typing import Any, Dict, List, Optional, Union +from typing import Dict, List, Optional, Union # Single source of truth for the default export dir. Kept in the current working # directory on purpose: an export is a user-requested artifact, so it lands next to @@ -88,9 +88,7 @@ def _peek_recipe_args() -> tuple: return _RECIPE_EXPORT, _RECIPE_EXPORT_DIR -from nvflare.apis.executor import Executor from nvflare.apis.filter import Filter -from nvflare.apis.impl.controller import Controller from nvflare.app_common.widgets.decomposer_reg import DecomposerRegister from nvflare.fuel.utils.fobs import Decomposer from nvflare.job_config.api import FedJob @@ -433,81 +431,6 @@ def add_client_file(self, file_path: str, clients: Optional[List[str]] = None): self._add_to_client_apps(file_path, clients=clients) - def add_client_component(self, component: Any, clients: Optional[List[str]] = None, id: Optional[str] = None): - """Add a component to client apps. - - Use this for client-side components such as tracking receivers or streamers, - instead of reaching into the recipe's generated job. The component is - registered in each targeted client app's configuration. - - This is a bounded placement primitive for components only. Files, config - parameters, filters, and executors have dedicated APIs: ``add_client_file``, - ``add_client_config``, ``add_client_input_filter``/``add_client_output_filter``, - and the recipe's own executor/train-script parameters. - - Args: - component: The component object to add to client apps. - clients: Optional list of specific client names. If None, applies to all clients. - Targeting specific clients requires the recipe's client apps to be per-site - (e.g. recipes constructed with the per_site_config constructor argument); - with the default all-clients topology, targeted placement raises ValueError - instead of silently dropping the component from the generated job. - id: Optional component id. If None, an id is generated. If the id is already - used in a client app, a numeric suffix is appended. - - Returns: - None. The final id may differ per client app (numeric suffixing is per app), - so no single id is returned; pass an explicit id that is unused in the - targeted apps if you need a stable reference. - - Raises: - TypeError: If component is a str, dict, Filter, Controller, Executor, or a - script runner, or clients is not a list. - ValueError: If clients is empty or names non-client sites, or targets specific - clients while the recipe's client app applies to all clients. - - Example: - # Add a streamer component to all clients - recipe.add_client_component(TensorClientStreamer()) - - # Add a tracking receiver to one site only (per-site client apps required) - recipe.add_client_component(receiver, clients=["site-1"], id="mlflow_receiver") - """ - self._validate_placed_component(component, "client") - self._add_to_client_apps(component, clients=clients, id=id) - - @staticmethod - def _validate_placed_component(component: Any, target_type: str) -> None: - """Reject objects that have dedicated APIs so component placement stays bounded.""" - if isinstance(component, str): - raise TypeError(f"component must be an object; use add_{target_type}_file for files or scripts") - if isinstance(component, dict): - raise TypeError(f"component must be an object; use add_{target_type}_config for configuration parameters") - if isinstance(component, Filter): - raise TypeError( - f"Filters are wired into filter chains; use add_{target_type}_input_filter or add_{target_type}_output_filter" - ) - # Check Controller before Executor so a hybrid subclass gets the accurate message. - if isinstance(component, Controller): - if target_type == "client": - raise TypeError( - "Controllers run in the server workflow and are configured by the recipe; they cannot be added to clients" - ) - raise TypeError( - "Controllers are configured by the recipe itself; they cannot be added through add_server_component" - ) - if isinstance(component, Executor): - raise TypeError( - f"Executors are configured through the recipe's executor/train-script parameters, not add_{target_type}_component" - ) - # Script runners are executor-installing wrappers, not plain components. - from nvflare.job_config.script_runner import BaseScriptRunner, ScriptRunner - - if isinstance(component, (BaseScriptRunner, ScriptRunner)): - raise TypeError( - f"script runners are configured through the recipe's train-script parameters, not add_{target_type}_component" - ) - def add_server_output_filter(self, filter: Filter, tasks: Optional[List[str]] = None): """Add a filter to the server for outgoing tasks to clients. @@ -567,39 +490,6 @@ def add_server_file(self, file_path: str): self.job.to_server(file_path) - def add_server_component(self, component: Any, id: Optional[str] = None) -> Any: - """Add a component to the server app. - - Use this for server-side components such as tracking receivers or streamers, - instead of reaching into the recipe's generated job. The component is - registered in the server app's configuration. - - This is a bounded placement primitive for components only. Files, config - parameters, and filters have dedicated APIs: ``add_server_file``, - ``add_server_config``, and ``add_server_input_filter``/``add_server_output_filter``. - Controllers are configured by the recipe itself. - - Args: - component: The component object to add to the server app. - id: Optional component id. If None, an id is generated. If the id is already - used in the server app, a numeric suffix is appended. - - Returns: - The final component id assigned in the server app. Components that implement - their own job-registration hook (``add_to_fed_job``) return that hook's result - instead, which may not be an id. - - Raises: - TypeError: If component is a str, dict, Filter, Controller, Executor, or a - script runner. - - Example: - # Add a streamer component to the server - streamer_id = recipe.add_server_component(TensorServerStreamer()) - """ - self._validate_placed_component(component, "server") - return self.job.to_server(component, id=id) - @staticmethod def _get_full_class_name(obj): """ diff --git a/nvflare/recipe/utils.py b/nvflare/recipe/utils.py index 86a70bc70a..2bb3c139b4 100644 --- a/nvflare/recipe/utils.py +++ b/nvflare/recipe/utils.py @@ -293,9 +293,9 @@ def add_experiment_tracking( client_config["events"] = [ANALYTIC_EVENT_TYPE] client_receiver = receiver_class(**client_config) - # Route through the recipe placement helper so existing per-site client apps + # Route through the recipe placement layer so existing per-site client apps # are preserved (to_clients would target ALL_SITES even when per-site apps exist). - recipe.add_client_component(client_receiver, clients=clients, id="client_receiver") + recipe._add_to_client_apps(client_receiver, clients=clients, id="client_receiver") def add_cross_site_evaluation( diff --git a/tests/unit_test/recipe/spec_test.py b/tests/unit_test/recipe/spec_test.py index 947d5d0bcd..19acd7929a 100644 --- a/tests/unit_test/recipe/spec_test.py +++ b/tests/unit_test/recipe/spec_test.py @@ -758,232 +758,59 @@ def __init__(self): set_per_site_config(BasicRecipe(), config) -class _PlainComponent: - """Minimal component for placement tests.""" +class TestClientPlacementHardening: + """Test _add_to_client_apps validation through the public clients=-taking helpers. - def __init__(self, tag: str = ""): - self.tag = tag + These guards fix pre-existing silent failures: targeting specific clients while + the job has an all-clients app used to silently drop the placement at export, + and malformed clients values were silently ignored or iterated per character. + """ - -class TestRecipeComponentPlacement: - """Test add_server_component and add_client_component methods in Recipe.""" - - def _make_recipe(self, name="test_component_placement"): + def _make_recipe(self, name="test_placement_hardening"): from nvflare.recipe.spec import Recipe return Recipe(FedJob(name=name, min_clients=1)) - def test_add_server_component_registers_and_returns_id(self): - recipe = self._make_recipe() - comp = _PlainComponent() - - cid = recipe.add_server_component(comp, id="my_comp") - - assert cid == "my_comp" - server_app = recipe.job._deploy_map.get("server") - assert server_app is not None - assert server_app.app_config.components[cid] is comp - - def test_add_server_component_generates_id_when_none(self): - recipe = self._make_recipe() - comp = _PlainComponent() - - cid = recipe.add_server_component(comp) - - assert cid - assert recipe.job._deploy_map["server"].app_config.components[cid] is comp - - def test_add_server_component_duplicate_id_gets_suffix(self): - recipe = self._make_recipe() - first = _PlainComponent("first") - second = _PlainComponent("second") - - cid1 = recipe.add_server_component(first, id="recv") - cid2 = recipe.add_server_component(second, id="recv") - - assert cid1 == "recv" - assert cid2 != cid1 - components = recipe.job._deploy_map["server"].app_config.components - assert components[cid1] is first - assert components[cid2] is second - - def test_add_client_component_all_clients(self): - from nvflare.apis.job_def import ALL_SITES - + def test_targeted_config_rejects_all_clients_topology(self): recipe = self._make_recipe() - comp = _PlainComponent() - - recipe.add_client_component(comp, id="recv") - - all_clients_app = recipe.job._deploy_map.get(ALL_SITES) - assert all_clients_app is not None - assert all_clients_app.app_config.components["recv"] is comp - - def test_add_client_component_specific_clients(self): - from nvflare.apis.job_def import ALL_SITES - - recipe = self._make_recipe() - comp = _PlainComponent() - - recipe.add_client_component(comp, clients=["site-1"], id="recv") - - assert recipe.job._deploy_map["site-1"].app_config.components["recv"] is comp - assert ALL_SITES not in recipe.job._deploy_map - assert "site-2" not in recipe.job._deploy_map - - def test_add_client_component_preserves_per_site_apps(self): - from nvflare.apis.job_def import ALL_SITES - - recipe = self._make_recipe() - # Existing per-site client topology. - recipe.job.to(_PlainComponent("site-1-only"), "site-1") - recipe.job.to(_PlainComponent("site-2-only"), "site-2") - - shared = _PlainComponent("shared") - recipe.add_client_component(shared, id="recv") - - # Added to every existing per-site app instead of creating an ALL_SITES app. - assert ALL_SITES not in recipe.job._deploy_map - for site in ("site-1", "site-2"): - components = recipe.job._deploy_map[site].app_config.components - assert any(c is shared for c in components.values()) - - @pytest.mark.parametrize( - "component, match", - [ - ("component.py", "use add_client_file"), - ({"param": 1}, "use add_client_config"), - ], - ) - def test_add_client_component_rejects_str_and_dict(self, component, match): - recipe = self._make_recipe() - with pytest.raises(TypeError, match=match): - recipe.add_client_component(component) - - @pytest.mark.parametrize( - "component, match", - [ - ("component.py", "use add_server_file"), - ({"param": 1}, "use add_server_config"), - ], - ) - def test_add_server_component_rejects_str_and_dict(self, component, match): - recipe = self._make_recipe() - with pytest.raises(TypeError, match=match): - recipe.add_server_component(component) - - def test_add_client_component_rejects_controller(self): - from nvflare.apis.impl.controller import Controller - - class _DummyController(Controller): - def control_flow(self, abort_signal, fl_ctx): - pass - - def start_controller(self, fl_ctx): - pass + # Default recipe topology: one client app for all clients. + recipe.job.to_clients({"executor_standin": True}) - def stop_controller(self, fl_ctx): - pass + with pytest.raises(ValueError, match="applies to all clients"): + recipe.add_client_config({"timeout": 600}, clients=["site-1"]) + def test_targeted_file_rejects_all_clients_topology(self, tmp_path): + src_file = tmp_path / "wrapper.sh" + src_file.write_text("#!/bin/sh\n") recipe = self._make_recipe() - with pytest.raises(TypeError, match="Controllers"): - recipe.add_client_component(_DummyController()) - - @pytest.mark.parametrize("method", ["add_client_component", "add_server_component"]) - def test_component_methods_reject_executor(self, method): - from nvflare.apis.executor import Executor + recipe.job.to_clients({"executor_standin": True}) - class _DummyExecutor(Executor): - def execute(self, task_name, shareable, fl_ctx, abort_signal): - return None - - recipe = self._make_recipe() - with pytest.raises(TypeError, match="Executors"): - getattr(recipe, method)(_DummyExecutor()) + with pytest.raises(ValueError, match="applies to all clients"): + recipe.add_client_file(str(src_file), clients=["site-1"]) - def test_add_client_component_specific_clients_rejects_all_sites_topology(self): + def test_targeted_config_works_with_per_site_topology(self): recipe = self._make_recipe() - # Default recipe topology: one client app for all clients. - recipe.job.to_clients(_PlainComponent("executor-standin")) + recipe.job.to({"site_arg": 1}, "site-1") + recipe.job.to({"site_arg": 2}, "site-2") - with pytest.raises(ValueError, match="applies to all clients"): - recipe.add_client_component(_PlainComponent(), clients=["site-1"]) + recipe.add_client_config({"timeout": 600}, clients=["site-1"]) - @pytest.mark.parametrize("bad_site", ["server", "@ALL"]) - def test_add_client_component_rejects_non_client_site_names(self, bad_site): - recipe = self._make_recipe() - with pytest.raises(ValueError, match="invalid client name"): - recipe.add_client_component(_PlainComponent(), clients=[bad_site]) + assert recipe.job._deploy_map["site-1"].app_config.additional_params["timeout"] == 600 + assert "timeout" not in recipe.job._deploy_map["site-2"].app_config.additional_params - def test_add_client_component_rejects_bare_string_clients(self): + def test_clients_must_be_a_list(self): recipe = self._make_recipe() # A bare string would otherwise iterate per character and create per-char apps. with pytest.raises(TypeError, match="must be a list"): - recipe.add_client_component(_PlainComponent(), clients="site-1") + recipe.add_client_config({"timeout": 600}, clients="site-1") - def test_add_client_component_rejects_empty_clients(self): + def test_clients_must_not_be_empty(self): recipe = self._make_recipe() with pytest.raises(ValueError, match="must not be empty"): - recipe.add_client_component(_PlainComponent(), clients=[]) - - @pytest.mark.parametrize("method", ["add_client_component", "add_server_component"]) - def test_component_methods_reject_filter(self, method): - from nvflare.apis.filter import Filter - - class _DummyFilter(Filter): - def process(self, shareable, fl_ctx): - return shareable - - recipe = self._make_recipe() - with pytest.raises(TypeError, match="input_filter"): - getattr(recipe, method)(_DummyFilter()) + recipe.add_client_config({"timeout": 600}, clients=[]) - def test_add_server_component_rejects_controller(self): - from nvflare.apis.impl.controller import Controller - - class _DummyController(Controller): - def control_flow(self, abort_signal, fl_ctx): - pass - - def start_controller(self, fl_ctx): - pass - - def stop_controller(self, fl_ctx): - pass - - recipe = self._make_recipe() - with pytest.raises(TypeError, match="Controllers"): - recipe.add_server_component(_DummyController()) - - @pytest.mark.parametrize("method", ["add_client_component", "add_server_component"]) - def test_component_methods_reject_script_runner(self, method): - from nvflare.job_config.script_runner import FrameworkType, ScriptRunner - - runner = ScriptRunner(script="train.py", framework=FrameworkType.RAW) + @pytest.mark.parametrize("bad_site", ["server", "@ALL"]) + def test_clients_must_name_client_sites(self, bad_site): recipe = self._make_recipe() - with pytest.raises(TypeError, match="script runners"): - getattr(recipe, method)(runner) - - def test_components_appear_in_exported_job_config(self, tmp_path): - recipe = self._make_recipe("test_component_export") - recipe.job.to_server({"server_arg": True}) - server_comp = _PlainComponent("server-side") - client_comp = _PlainComponent("client-side") - - recipe.add_server_component(server_comp, id="server_recv") - recipe.add_client_component(client_comp, id="client_recv") - - recipe.job.export_job(str(tmp_path)) - job_dir = tmp_path / "test_component_export" - - with open(job_dir / "app" / "config" / "config_fed_server.json") as f: - server_cfg = json.load(f) - with open(job_dir / "app" / "config" / "config_fed_client.json") as f: - client_cfg = json.load(f) - - server_entry = next(c for c in server_cfg["components"] if c["id"] == "server_recv") - assert server_entry["path"].endswith("_PlainComponent") - assert server_entry["args"] == {"tag": "server-side"} - client_entry = next(c for c in client_cfg["components"] if c["id"] == "client_recv") - assert client_entry["path"].endswith("_PlainComponent") - assert client_entry["args"] == {"tag": "client-side"} + with pytest.raises(ValueError, match="invalid client name"): + recipe.add_client_config({"timeout": 600}, clients=[bad_site]) From 88c972092500c4ee9281d4d94a0dd23d9851a992 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 15:48:58 -0700 Subject: [PATCH 5/5] Reject unknown site names in targeted client placement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When per-site client apps exist, targeting a site with no app silently created a bare, executor-less app for that site in the exported job — the same class of quiet misconfiguration the earlier topology guard turns into loud errors. _add_to_client_apps now validates that every name in clients matches an existing per-site client app when per-site apps exist, listing the known sites in the error. Creating apps from scratch on an empty deploy map remains permitted. Per-site tracking tests now pre-seed per-site apps, mirroring real recipes where apps exist (e.g. via per_site_config) before tracking is added. --- .../data_scientist_guide/recipe_api.rst | 6 ++-- nvflare/recipe/spec.py | 29 ++++++++++++++----- nvflare/recipe/utils.py | 5 ++-- tests/unit_test/recipe/spec_test.py | 8 +++++ tests/unit_test/recipe/utils_test.py | 21 ++++++++++++++ 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/docs/user_guide/data_scientist_guide/recipe_api.rst b/docs/user_guide/data_scientist_guide/recipe_api.rst index f0788e18c7..69c1f2fc2f 100644 --- a/docs/user_guide/data_scientist_guide/recipe_api.rst +++ b/docs/user_guide/data_scientist_guide/recipe_api.rst @@ -100,9 +100,11 @@ File packaging helpers: Helpers that accept ``clients`` target specific generated client apps. This requires per-site client apps: construct the recipe with the -``per_site_config`` constructor argument on recipes that support it. With the +``per_site_config`` constructor argument on recipes that support it, and each +name in ``clients`` must match an existing per-site client app. With the default all-clients topology, targeted calls raise an error rather than -silently dropping the change from the generated job. Calling +silently dropping the change from the generated job, and unknown site names +raise an error rather than deploying a bare app to that site. Calling ``set_per_site_config`` after construction records the configuration for ``configured_sites()`` but does not yet rebuild an existing all-clients app into per-site apps; recipes will interpret helper-provided per-site config as diff --git a/nvflare/recipe/spec.py b/nvflare/recipe/spec.py index 9e54c4c4ab..63e9006e88 100644 --- a/nvflare/recipe/spec.py +++ b/nvflare/recipe/spec.py @@ -317,9 +317,12 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs Raises: TypeError: If clients is not a list. - ValueError: If clients is empty or contains a non-client name, or if specific + ValueError: If clients is empty or contains a non-client name; if specific clients are targeted while the recipe's client app applies to all clients - (per-site placement cannot be expressed in the generated job in that topology). + (per-site placement cannot be expressed in the generated job in that + topology); or if clients names a site with no existing client app while + per-site client apps exist (that would deploy a bare, executor-less app + to that site). """ from nvflare.apis.job_def import ALL_SITES, SERVER_SITE_NAME from nvflare.job_config.defs import JobTargetType @@ -327,13 +330,13 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs # FedJob has no public API to list per-site deploy targets, so we inspect # private deploy map to preserve existing per-site client topology. deploy_map = getattr(self.job, "_deploy_map", {}) + existing_client_sites = [ + target + for target in deploy_map.keys() + if target not in [ALL_SITES, SERVER_SITE_NAME] + and JobTargetType.get_target_type(target) == JobTargetType.CLIENT + ] if clients is None: - existing_client_sites = [ - target - for target in deploy_map.keys() - if target not in [ALL_SITES, SERVER_SITE_NAME] - and JobTargetType.get_target_type(target) == JobTargetType.CLIENT - ] if existing_client_sites: for site in existing_client_sites: self.job.to(obj, site, **kwargs) @@ -358,6 +361,16 @@ def _add_to_client_apps(self, obj, clients: Optional[List[str]] = None, **kwargs "Construct the recipe with per-site client apps (e.g. the per_site_config constructor " "argument on recipes that support it) or omit clients to apply to all clients." ) + if existing_client_sites: + # Targeting a site with no app would create a bare, executor-less app for + # that site in the exported job — the same class of quiet misconfiguration + # as the ALL_SITES case above, so fail loudly instead. + unknown = [c for c in clients if c not in existing_client_sites] + if unknown: + raise ValueError( + f"unknown client site(s) {unknown}: this recipe has per-site client apps " + f"only for {sorted(existing_client_sites)}" + ) for client in clients: self.job.to(obj, client, **kwargs) diff --git a/nvflare/recipe/utils.py b/nvflare/recipe/utils.py index 2bb3c139b4..763133b919 100644 --- a/nvflare/recipe/utils.py +++ b/nvflare/recipe/utils.py @@ -231,8 +231,9 @@ def add_experiment_tracking( To give sites different receiver configs (e.g. per-site tracking_uri), call this function once per site with that site's tracking_config and clients=[site]. Targeting specific clients requires the recipe's client apps to be per-site - (e.g. recipes constructed with the per_site_config constructor argument); - with the default all-clients topology, targeted placement raises ValueError. + (e.g. recipes constructed with the per_site_config constructor argument), and + each name must match an existing per-site client app; with the default + all-clients topology or unknown site names, targeted placement raises ValueError. Examples: # Server-side tracking (default - federated metrics) diff --git a/tests/unit_test/recipe/spec_test.py b/tests/unit_test/recipe/spec_test.py index 19acd7929a..d8c1730858 100644 --- a/tests/unit_test/recipe/spec_test.py +++ b/tests/unit_test/recipe/spec_test.py @@ -798,6 +798,14 @@ def test_targeted_config_works_with_per_site_topology(self): assert recipe.job._deploy_map["site-1"].app_config.additional_params["timeout"] == 600 assert "timeout" not in recipe.job._deploy_map["site-2"].app_config.additional_params + def test_targeted_config_rejects_unknown_site_with_per_site_topology(self): + recipe = self._make_recipe() + recipe.job.to({"site_arg": 1}, "site-1") + recipe.job.to({"site_arg": 2}, "site-2") + + with pytest.raises(ValueError, match=r"unknown client site.*site-3"): + recipe.add_client_config({"timeout": 600}, clients=["site-3"]) + def test_clients_must_be_a_list(self): recipe = self._make_recipe() # A bare string would otherwise iterate per character and create per-char apps. diff --git a/tests/unit_test/recipe/utils_test.py b/tests/unit_test/recipe/utils_test.py index 6391a58fde..73ec1f8069 100644 --- a/tests/unit_test/recipe/utils_test.py +++ b/tests/unit_test/recipe/utils_test.py @@ -570,6 +570,10 @@ def test_client_side_tracking_per_site_configs(self, dummy_tracking): from nvflare.recipe.utils import add_experiment_tracking recipe = self._make_recipe() + # Real recipes have per-site client apps (e.g. from per_site_config) before + # tracking is added; targeting only existing sites is enforced. + for site in ("site-1", "site-2"): + recipe.job.to({"site_arg": site}, site) for site in ("site-1", "site-2"): add_experiment_tracking( recipe, @@ -629,6 +633,23 @@ def test_clients_empty_list_raises(self, dummy_tracking): with pytest.raises(ValueError, match="must not be empty"): add_experiment_tracking(recipe, dummy_tracking, {"tracking_uri": "u"}, client_side=True, clients=[]) + def test_clients_targeting_rejects_unknown_site(self, dummy_tracking): + from nvflare.recipe.utils import add_experiment_tracking + + recipe = self._make_recipe() + recipe.job.to({"site_arg": 1}, "site-1") + recipe.job.to({"site_arg": 2}, "site-2") + + with pytest.raises(ValueError, match="unknown client site"): + add_experiment_tracking( + recipe, + dummy_tracking, + {"tracking_uri": "u"}, + client_side=True, + server_side=False, + clients=["site-3"], + ) + def test_clients_targeting_rejects_all_sites_topology(self, dummy_tracking): from nvflare.recipe.utils import add_experiment_tracking