From 51dd8928cf1c50108b4d1bb7363c836b6cb62b60 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Thu, 2 Jul 2026 09:35:09 -0700 Subject: [PATCH 01/12] Add ClientAPIExecutor skeleton and backend spec EX-2 (Wave 0, interface freeze #2) of the Client API Execution Modes program (see docs/design/client_api_execution_modes_plan.md on PR #4853). New public ClientAPIExecutor at nvflare/app_common/executors/ client_api_executor.py with execution_mode dispatch (in_process / external_process / attach), plus a ClientAPIBackendSpec ABC and a frozen ClientAPIBackendContext the executor hands each backend (config + a back-reference for analytics). The three backends land in follow-up PRs (in_process, external_process, attach); until then each mode fails the job cleanly via system_panic rather than hanging. The constructor is the frozen configuration surface for the program: mode-scoped args are validated symmetrically (an arg set for a mode that ignores it is rejected with a clear error, not silently dropped). Beyond the design's V1 Configuration Surface list it also carries task_script_path/task_script_args (in_process trainer entry point), train/evaluate/submit_model_task_name + train_with_evaluation (power flare.is_train()/is_evaluate()/is_submit_model()), and memory_gc_rounds/ cuda_empty_cache, all forwarded from today's InProcessClientAPIExecutor/ ScriptRunner so existing jobs map without loss. The design doc's Configuration Surface and disposition table are updated to match. Analytics: the executor owns LOG-to-analytics conversion; the local path fires the un-prefixed event + ConvertToFedEvent (today's in-process behavior), the fed path fires "fed.analytix_log_stats" (today's MetricRelay behavior). UnsafeJobError propagates out of execute() so ClientRunner's UNSAFE_JOB handling still fires. No existing class is modified. --- .../executors/client_api/__init__.py | 13 + .../executors/client_api/backend_spec.py | 196 ++++++ .../executors/client_api_executor.py | 474 ++++++++++++++ .../executors/client_api_executor_test.py | 577 ++++++++++++++++++ 4 files changed, 1260 insertions(+) create mode 100644 nvflare/app_common/executors/client_api/__init__.py create mode 100644 nvflare/app_common/executors/client_api/backend_spec.py create mode 100644 nvflare/app_common/executors/client_api_executor.py create mode 100644 tests/unit_test/app_common/executors/client_api_executor_test.py diff --git a/nvflare/app_common/executors/client_api/__init__.py b/nvflare/app_common/executors/client_api/__init__.py new file mode 100644 index 0000000000..4fc25d0d3c --- /dev/null +++ b/nvflare/app_common/executors/client_api/__init__.py @@ -0,0 +1,13 @@ +# Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. diff --git a/nvflare/app_common/executors/client_api/backend_spec.py b/nvflare/app_common/executors/client_api/backend_spec.py new file mode 100644 index 0000000000..c40801ea96 --- /dev/null +++ b/nvflare/app_common/executors/client_api/backend_spec.py @@ -0,0 +1,196 @@ +# Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Backend spec for the Client API execution modes (V1-internal). + +Design: docs/design/client_api_execution_modes.md ("Overview", "Execution Modes", +"Client API Backends"). One ClientAPIExecutor delegates to one mode-specific backend: + +- in_process: trainer runs inside the Client Job (CJ) process over DataBus (EX-3) +- external_process: NVFlare launches and owns the trainer process tree over Cell (EP-4) +- attach: an externally owned trainer attaches over Cell (AT-2) + +This module is internal to NVFlare. It is not a user extension point; users configure +``ClientAPIExecutor(execution_mode=...)`` only. +""" + +from abc import ABC, abstractmethod +from dataclasses import dataclass +from typing import TYPE_CHECKING, Optional + +from nvflare.apis.fl_context import FLContext +from nvflare.apis.shareable import Shareable +from nvflare.apis.signal import Signal + +if TYPE_CHECKING: + # Import for typing only; avoids a runtime import cycle + # (client_api_executor imports this module to build the context). + from nvflare.app_common.executors.client_api_executor import ClientAPIExecutor + + +@dataclass(frozen=True) +class ClientAPIBackendContext: + """Immutable config a ClientAPIExecutor hands to its backend at ``initialize()``. + + Rationale (finding 11): the executor's frozen constructor args live in private attributes and + the backend factories are zero-arg, so a backend previously had no clean way to read the + heartbeat/timeout/converter/task-name config it needs, nor a supported reference back to the + executor's analytics hook. This frozen snapshot is that supported channel - a backend reads its + config from here rather than reaching into ``ClientAPIExecutor`` private attributes. + + The fields mirror the frozen ``ClientAPIExecutor`` constructor surface one-to-one. ``executor`` + is a back-reference so a backend can: + + - call ``executor.fire_log_analytics(fl_ctx, dxo)`` for every trainer LOG message (the single + LOG-to-analytics ownership point; see design "Configuration Surface"), and + - select the federation-scoped analytics path when appropriate by setting + ``executor._analytics_fire_fed_event = True`` in ``initialize()`` (Cell backends do this when + no ConvertToFedEvent widget is configured), and + - use the executor's FLComponent logging helpers. + """ + + executor: "ClientAPIExecutor" + execution_mode: str + # in_process entry point + task_script_path: Optional[str] = None + task_script_args: str = "" + # external_process launch + command: Optional[str] = None + launch_once: bool = True + launch_timeout: Optional[float] = None + shutdown_timeout: Optional[float] = None + stop_grace_period: float = 30.0 + # session / protocol (out-of-process) + heartbeat_interval: float = 5.0 + heartbeat_timeout: float = 30.0 + task_wait_timeout: Optional[float] = None + result_wait_timeout: Optional[float] = None + # data / params + params_exchange_format: str = "numpy" + params_transfer_type: str = "FULL" + server_expected_format: str = "numpy" + from_nvflare_converter_id: Optional[str] = None + to_nvflare_converter_id: Optional[str] = None + # task-name / rank contract (all modes) + train_task_name: str = "train" + evaluate_task_name: str = "validate" + submit_model_task_name: str = "submit_model" + train_with_evaluation: bool = False + # memory management (all modes) + memory_gc_rounds: int = 0 + cuda_empty_cache: bool = False + # attach + attach_timeout: Optional[float] = None + allow_reconnect: bool = False + + +class ClientAPIBackendSpec(ABC): + """The narrow lifecycle contract that ClientAPIExecutor drives on its backend. + + Lifecycle ownership per execution mode: + + - in_process: the backend runs the trainer inside the CJ process and owns its thread. + - external_process: the backend launches and owns the external trainer process tree; it must + not stop the trainer before the payload transfer of a pending result reaches terminal state. + - attach: the external system owns the trainer process; the backend owns only the attach + session, token validation, and heartbeat lease. + """ + + @abstractmethod + def initialize(self, context: ClientAPIBackendContext, fl_ctx: FLContext) -> None: + """Prepares the backend for the run. Called once when the executor handles START_RUN. + + ``context`` (finding 11) is the frozen snapshot of the executor's configuration plus a + back-reference to the executor (for ``fire_log_analytics`` and logging). A backend should + read all of its config from ``context`` rather than from executor private attributes, and + should retain what it needs for ``execute``/``handle_event``/``finalize``. + + The backend sets up its control plane here (DataBus wiring for in_process; Cell session + machinery, bootstrap config, and - per launch_once policy - trainer launch for + external_process; attach listener/token for attach). + + Contract: raise an exception on any setup failure. The executor converts the exception + into system_panic so the job fails cleanly instead of hanging while tasks wait on a + backend that never became ready. + + Cleanup-on-failure contract (finding 12): ``initialize()`` must be exception-safe and + self-unwinding - if it raises, it must first release any partial setup it already made + (threads started, processes launched, listeners/tokens registered, files written). The + executor does NOT call ``finalize()`` on a backend whose ``initialize()`` raised, because + ``finalize()`` cannot assume a consistently half-initialized backend. Own your own rollback. + + Args: + context: the frozen backend configuration and executor back-reference. + fl_ctx: the FLContext of the START_RUN event. + """ + pass + + @abstractmethod + def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort_signal: Signal) -> Shareable: + """Executes one task on the trainer and returns its result. + + The backend delivers the task to the trainer (TASK_READY over Cell, or DataBus for + in_process), waits for the result within the executor-configured task/result bounds, and + returns the result Shareable. + + Contract: this method must always return a Shareable and must not hang past abort: + when abort_signal is triggered, the backend notifies/stops the trainer per its mode's + lifecycle ownership and returns ``make_reply(ReturnCode.TASK_ABORTED)``. On failure it + should return an error reply (e.g. ReturnCode.EXECUTION_EXCEPTION) rather than raise; + exceptions that do escape are converted to EXECUTION_EXCEPTION replies by the executor, + except UnsafeJobError which the executor lets propagate so ClientRunner can apply its + dedicated UNSAFE_JOB handling. + + Args: + task_name: name of the task. + shareable: the task data. + fl_ctx: the FLContext of the task. + abort_signal: checked during execution; triggered means the task is aborted. + + Returns: + The result Shareable (an error reply on failure/abort - never None). + """ + pass + + @abstractmethod + def handle_event(self, event_type: str, fl_ctx: FLContext) -> None: + """Handles an FL event relayed by the executor. + + The executor relays events other than START_RUN/END_RUN (those are mapped to + initialize/finalize). Backends use this for mode-specific bookkeeping. + + Contract: must not raise; log and continue on internal errors. + + Args: + event_type: the fired event type. + fl_ctx: the FLContext of the event. + """ + pass + + @abstractmethod + def finalize(self, fl_ctx: FLContext) -> None: + """Releases backend resources. Called when the executor handles END_RUN. + + The backend tears down per its mode's lifecycle ownership: stop the in-process trainer + thread; send SHUTDOWN and stop the owned process tree (honoring the executor's + shutdown_timeout and stop_grace_period, and pending payload terminal state) for + external_process; close the session lease (without killing the trainer) for attach. + + Contract: must be idempotent and must not raise. Not called if ``initialize()`` raised + (see the cleanup-on-failure contract on ``initialize()``). + + Args: + fl_ctx: the FLContext of the END_RUN event. + """ + pass diff --git a/nvflare/app_common/executors/client_api_executor.py b/nvflare/app_common/executors/client_api_executor.py new file mode 100644 index 0000000000..b156737624 --- /dev/null +++ b/nvflare/app_common/executors/client_api_executor.py @@ -0,0 +1,474 @@ +# Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""The public Client API executor, configured by execution mode. + +Design: docs/design/client_api_execution_modes.md ("What We Propose", "Overview", +"Execution Modes", "Configuration Surface"). This module path is normative - job configs +reference ``nvflare.app_common.executors.client_api_executor.ClientAPIExecutor``. + +This is the interface-freeze skeleton (plan: EX-2). The constructor surface below is frozen; +the mode backends land in follow-up PRs (in_process, external_process, attach). + +Divergence from the design's V1 "Configuration Surface" list +------------------------------------------------------------ +The design's V1 arg list is a subset; the frozen surface below adds the following load-bearing +args that the mode backends require and that both legacy executors +(InProcessClientAPIExecutor / ClientAPILauncherExecutor) already expose. They are recorded here +so the design's Configuration Surface can be synced separately: + +- ``task_script_path`` / ``task_script_args`` - in_process script entry point (the in_process + backend runs a user script via TaskScriptRunner; ``command`` names the external_process + trainer, ``task_script_path`` names the in_process one). +- ``train_task_name`` / ``evaluate_task_name`` / ``submit_model_task_name`` / + ``train_with_evaluation`` - power the rank-contract APIs flare.is_train()/is_evaluate()/ + is_submit_model(). +- ``memory_gc_rounds`` / ``cuda_empty_cache`` - periodic GC / CUDA cache management + (public on ScriptRunner and both legacy executors). +""" + +from typing import Callable, Dict, Optional + +from nvflare.apis.analytix import ANALYTIC_EVENT_TYPE +from nvflare.apis.dxo import DXO +from nvflare.apis.event_type import EventType +from nvflare.apis.executor import Executor +from nvflare.apis.fl_constant import ReturnCode +from nvflare.apis.fl_context import FLContext +from nvflare.apis.fl_exception import UnsafeJobError +from nvflare.apis.shareable import Shareable, make_reply +from nvflare.apis.signal import Signal +from nvflare.apis.utils.analytix_utils import send_analytic_dxo +from nvflare.app_common.app_constant import AppConstants +from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext, ClientAPIBackendSpec +from nvflare.app_common.widgets.convert_to_fed_event import FED_EVENT_PREFIX +from nvflare.client.config import ExchangeFormat, TransferType +from nvflare.security.logging import secure_format_exception, secure_format_traceback + + +class ExecutionMode: + """Valid values for ClientAPIExecutor's execution_mode (see design "Overview").""" + + IN_PROCESS = "in_process" + EXTERNAL_PROCESS = "external_process" + ATTACH = "attach" + + +ALL_EXECUTION_MODES = (ExecutionMode.IN_PROCESS, ExecutionMode.EXTERNAL_PROCESS, ExecutionMode.ATTACH) + +# Federation-scoped analytics event, matching MetricRelay's ex-process default +# (job_config/script_runner.py) and flower_job.py: "fed.analytix_log_stats". +FED_ANALYTIC_EVENT_TYPE = FED_EVENT_PREFIX + ANALYTIC_EVENT_TYPE + +# Frozen defaults for mode-specific knobs (design "Configuration Surface"). Kept as named +# constants so the constructor default and the wrong-mode "explicitly set a non-default" checks +# below cannot drift apart. +_DEFAULT_LAUNCH_ONCE = True +_DEFAULT_STOP_GRACE_PERIOD = 30.0 +_DEFAULT_HEARTBEAT_INTERVAL = 5.0 +_DEFAULT_HEARTBEAT_TIMEOUT = 30.0 + + +class ClientAPIExecutor(Executor): + """One executor for all Client API execution modes. + + The trainer-facing Client API (flare.init/receive/send/log) is unchanged; this executor + replaces the Pipe/launcher integration stack. It delegates to an internal mode-specific + backend (ClientAPIBackendSpec) resolved from ``execution_mode`` at START_RUN: + + - ``in_process``: trainer runs inside the CJ process over DataBus. + - ``external_process``: NVFlare launches and owns the trainer process tree; control over Cell. + - ``attach``: an externally started/owned trainer attaches over Cell. + """ + + def __init__( + self, + execution_mode: str, + command: Optional[str] = None, + task_script_path: Optional[str] = None, + task_script_args: str = "", + launch_once: bool = _DEFAULT_LAUNCH_ONCE, + launch_timeout: Optional[float] = None, + shutdown_timeout: Optional[float] = None, + stop_grace_period: float = _DEFAULT_STOP_GRACE_PERIOD, + heartbeat_interval: float = _DEFAULT_HEARTBEAT_INTERVAL, + heartbeat_timeout: float = _DEFAULT_HEARTBEAT_TIMEOUT, + task_wait_timeout: Optional[float] = None, + result_wait_timeout: Optional[float] = None, + params_exchange_format: str = ExchangeFormat.NUMPY, + params_transfer_type: str = TransferType.FULL, + server_expected_format: str = ExchangeFormat.NUMPY, + from_nvflare_converter_id: Optional[str] = None, + to_nvflare_converter_id: Optional[str] = None, + train_task_name: str = AppConstants.TASK_TRAIN, + evaluate_task_name: str = AppConstants.TASK_VALIDATION, + submit_model_task_name: str = AppConstants.TASK_SUBMIT_MODEL, + train_with_evaluation: bool = False, + memory_gc_rounds: int = 0, + cuda_empty_cache: bool = False, + attach_timeout: Optional[float] = None, + allow_reconnect: bool = False, + ): + """Initializes the ClientAPIExecutor. + + This constructor surface is frozen (design: "Configuration Surface"); parameter renames + break the surface-freeze test and downstream backend PRs. See the module docstring for the + args added beyond the design's V1 Configuration Surface list. + + Args: + execution_mode (str): One of "in_process", "external_process", or "attach". Required. + command (Optional[str]): The trainer launch command, e.g. "python custom/train.py", + "torchrun ...". Required for (and only valid in) "external_process" mode. An + empty/whitespace-only string is treated as unset. + task_script_path (Optional[str]): in_process only. Path to the user training script the + in_process backend runs via TaskScriptRunner. An empty/whitespace-only string is + treated as unset. (The in_process backend validates presence and ".py" suffix.) + task_script_args (str): in_process only. Arguments appended to task_script_path. + launch_once (bool): external_process only. Launch the trainer once per job (default) + vs once per task. + launch_timeout (Optional[float]): external_process only. Bound for the launched + trainer to complete its HELLO/session setup (this replaces the legacy + external_pre_init_timeout). None means no timeout. + shutdown_timeout (Optional[float]): external_process only. How long to wait for the + trainer to exit naturally after an orderly SHUTDOWN before starting forced + process-tree termination. None means the backend default. + stop_grace_period (float): external_process only. Grace period between SIGTERM and + SIGKILL when terminating the trainer process group (design: "Process-tree + termination"). + heartbeat_interval (float): out-of-process only (external_process/attach). Interval + (seconds) for session heartbeats. + heartbeat_timeout (float): out-of-process only (external_process/attach). Session lease + timeout (seconds) on missed heartbeats. An in-flight payload transfer keeps the + lease alive (design: "Heartbeat and Liveness"). + task_wait_timeout (Optional[float]): Bound for the trainer to accept a delivered + task. None means no timeout. + result_wait_timeout (Optional[float]): Control-side bound for retrieving the task + result. Payload transfer completion is governed by the shared transfer layer, + not by this value. None means no timeout. + params_exchange_format (str): Format to exchange parameters with the training + script, e.g. "pytorch", "numpy", "raw" (see ExchangeFormat). + params_transfer_type (str): "FULL" (whole model) or "DIFF" (difference only). + server_expected_format (str): Format to exchange parameters between server and + client (see ExchangeFormat). + from_nvflare_converter_id (Optional[str]): Component id of the ParamsConverter + applied to task data sent from the NVFlare controller side to the trainer side. + to_nvflare_converter_id (Optional[str]): Component id of the ParamsConverter + applied to results sent from the trainer side to the NVFlare controller side. + train_task_name (str): Task name treated as "train" by flare.is_train() (rank + contract). Defaults to AppConstants.TASK_TRAIN. + evaluate_task_name (str): Task name treated as "evaluate" by flare.is_evaluate(). + Defaults to AppConstants.TASK_VALIDATION. + submit_model_task_name (str): Task name treated as "submit_model" by + flare.is_submit_model(). Defaults to AppConstants.TASK_SUBMIT_MODEL. + train_with_evaluation (bool): Whether the trainer also returns evaluation metrics with + the trained model. + memory_gc_rounds (int): Force a GC cycle every N rounds (0 disables). + cuda_empty_cache (bool): Whether to also empty the CUDA cache during memory cleanup. + attach_timeout (Optional[float]): attach only. Bound for the externally started + trainer to attach. None means no timeout. + allow_reconnect (bool): attach only. Whether a trainer may re-attach to an existing + session after a disconnect. + """ + super().__init__() + + if execution_mode not in ALL_EXECUTION_MODES: + raise ValueError(f"invalid execution_mode {execution_mode!r}: must be one of {list(ALL_EXECUTION_MODES)}") + + # Normalize an empty/whitespace command or task_script_path to None up front so "" means + # "unset" uniformly in every mode. Previously external_process used `if not command` while + # the other modes used `command is not None`, so command="" was rejected with a misleading + # "only valid for external_process" message in in_process/attach instead of treated as unset. + command = self._normalize_optional_str(command) + task_script_path = self._normalize_optional_str(task_script_path) + + is_in_process = execution_mode == ExecutionMode.IN_PROCESS + is_external = execution_mode == ExecutionMode.EXTERNAL_PROCESS + is_attach = execution_mode == ExecutionMode.ATTACH + + # --- external_process command / in_process script entry point --- + if is_external: + if not command: + raise ValueError( + "execution_mode 'external_process' requires a non-empty command " + "(e.g. command='python custom/train.py')" + ) + elif command is not None: + raise self._wrong_mode_error("command", command, "'external_process'", execution_mode) + + if not is_in_process: + if task_script_path is not None: + raise self._wrong_mode_error("task_script_path", task_script_path, "'in_process'", execution_mode) + if task_script_args: + raise self._wrong_mode_error("task_script_args", task_script_args, "'in_process'", execution_mode) + + # --- external_process-only lifecycle knobs (reject only when explicitly set away from the + # frozen default in a mode that ignores them) --- + if not is_external: + if launch_once != _DEFAULT_LAUNCH_ONCE: + raise self._wrong_mode_error("launch_once", launch_once, "'external_process'", execution_mode) + if launch_timeout is not None: + raise self._wrong_mode_error("launch_timeout", launch_timeout, "'external_process'", execution_mode) + if shutdown_timeout is not None: + raise self._wrong_mode_error("shutdown_timeout", shutdown_timeout, "'external_process'", execution_mode) + if stop_grace_period != _DEFAULT_STOP_GRACE_PERIOD: + raise self._wrong_mode_error( + "stop_grace_period", stop_grace_period, "'external_process'", execution_mode + ) + + # --- heartbeat knobs are out-of-process only (there is no session heartbeat in_process) --- + if is_in_process: + if heartbeat_interval != _DEFAULT_HEARTBEAT_INTERVAL: + raise self._wrong_mode_error( + "heartbeat_interval", heartbeat_interval, "'external_process' or 'attach'", execution_mode + ) + if heartbeat_timeout != _DEFAULT_HEARTBEAT_TIMEOUT: + raise self._wrong_mode_error( + "heartbeat_timeout", heartbeat_timeout, "'external_process' or 'attach'", execution_mode + ) + + # --- attach-only knobs --- + if not is_attach: + if attach_timeout is not None: + raise self._wrong_mode_error("attach_timeout", attach_timeout, "'attach'", execution_mode) + # Only reject a truthy allow_reconnect: allow_reconnect=False (the frozen default) is + # indistinguishable from "not set". Using `if allow_reconnect` (not `is not False`) + # also stops misfires on falsy-but-not-False values (None, 0, numpy.bool_(False)). + if allow_reconnect: + raise self._wrong_mode_error("allow_reconnect", allow_reconnect, "'attach'", execution_mode) + + self._execution_mode = execution_mode + self._command = command + self._task_script_path = task_script_path + self._task_script_args = task_script_args + self._launch_once = launch_once + self._launch_timeout = launch_timeout + self._shutdown_timeout = shutdown_timeout + self._stop_grace_period = stop_grace_period + self._heartbeat_interval = heartbeat_interval + self._heartbeat_timeout = heartbeat_timeout + self._task_wait_timeout = task_wait_timeout + self._result_wait_timeout = result_wait_timeout + self._params_exchange_format = params_exchange_format + self._params_transfer_type = params_transfer_type + self._server_expected_format = server_expected_format + self._from_nvflare_converter_id = from_nvflare_converter_id + self._to_nvflare_converter_id = to_nvflare_converter_id + self._train_task_name = train_task_name + self._evaluate_task_name = evaluate_task_name + self._submit_model_task_name = submit_model_task_name + self._train_with_evaluation = train_with_evaluation + self._memory_gc_rounds = memory_gc_rounds + self._cuda_empty_cache = cuda_empty_cache + self._attach_timeout = attach_timeout + self._allow_reconnect = allow_reconnect + + self._backend: Optional[ClientAPIBackendSpec] = None + + # Analytics-event ownership (design: "Configuration Surface" - "the executor's Cell + # backend converts [LOG messages] into fed.analytix_log_stats analytics events"). + # False (default): fire the local un-prefixed ANALYTIC_EVENT_TYPE and rely on a + # ConvertToFedEvent widget (added by BaseFedJob) to re-fire it as + # "fed.analytix_log_stats" - today's in-process executor behavior. + # True: fire a federation-scoped event directly (today's MetricRelay behavior for + # ex-process). Cell backends (EP-4/AT-2) may select this path in initialize(). + self._analytics_fire_fed_event: bool = False + + def handle_event(self, event_type: str, fl_ctx: FLContext): + if event_type == EventType.START_RUN: + super().handle_event(event_type, fl_ctx) + try: + self._backend = self._create_backend() + self._backend.initialize(self._build_backend_context(), fl_ctx) + except Exception as e: + # finding 12: initialize() is contracted to self-unwind its partial setup on + # failure, so the executor does NOT call finalize() on a half-initialized backend; + # it just drops the reference and panics so the job fails cleanly. + self._backend = None + self.log_error(fl_ctx, secure_format_traceback(), fire_event=False) + self.system_panic( + f"ClientAPIExecutor cannot start: backend for execution_mode " + f"'{self._execution_mode}' failed to initialize: {secure_format_exception(e)}", + fl_ctx, + ) + elif event_type == EventType.END_RUN: + backend = self._backend + self._backend = None + if backend is not None: + try: + backend.finalize(fl_ctx) + except Exception: + self.log_error(fl_ctx, secure_format_traceback(), fire_event=False) + super().handle_event(event_type, fl_ctx) + else: + if self._backend is not None: + try: + self._backend.handle_event(event_type, fl_ctx) + except Exception: + self.log_error(fl_ctx, secure_format_traceback(), fire_event=False) + super().handle_event(event_type, fl_ctx) + + def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort_signal: Signal) -> Shareable: + backend = self._backend + if backend is None: + # START_RUN either never happened or backend initialization failed (and the executor + # already panicked). Reply with an error instead of waiting on a backend that will + # never be ready. + self.log_error( + fl_ctx, + f"no Client API backend available for execution_mode '{self._execution_mode}' - " + f"backend initialization failed or START_RUN was not handled", + ) + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + + try: + result = backend.execute(task_name, shareable, fl_ctx, abort_signal) + except UnsafeJobError: + # ClientRunner has dedicated handling for UnsafeJobError (client_runner.py maps it + # to ReturnCode.UNSAFE_JOB and marks the job unsafe). Do NOT swallow it into a + # generic EXECUTION_EXCEPTION reply here - let it propagate so that handling fires. + # (UnsafeComponentError has no such special handling and is left to the generic + # branch below, which is its existing behavior.) + raise + except Exception: + self.log_error(fl_ctx, secure_format_traceback()) + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + + if not isinstance(result, Shareable): + self.log_error(fl_ctx, f"bad result from backend: expected Shareable but got {type(result)}") + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + return result + + def fire_log_analytics(self, fl_ctx: FLContext, dxo: DXO) -> None: + """Converts trainer LOG data into an analytics event. Executor-owned surface. + + Backends call this for every LOG control message (flare.log from the trainer), + regardless of execution mode - this replaces MetricRelay (ex-process) and the + in-process executor's log callback as the single analytics-event ownership point. + + The fire path is mode-selectable via ``self._analytics_fire_fed_event``: + + - False (default): fires the local, un-prefixed ANALYTIC_EVENT_TYPE + ("analytix_log_stats") and relies on the ConvertToFedEvent widget (added by + BaseFedJob) to forward it to the server as "fed.analytix_log_stats". + - True: fires a federation-scoped event directly to the server, matching what + MetricRelay does today for ex-process metrics. Cell backends may select this + path during initialize() when no ConvertToFedEvent widget is configured. + + The two paths must land on the same server-side event name. ConvertToFedEvent prefixes + the local event with "fed.", so the fed path must fire the already-prefixed + FED_ANALYTIC_EVENT_TYPE ("fed.analytix_log_stats"); firing the un-prefixed name + federation-scoped would miss every consumer listening on "fed.analytix_log_stats" + (MetricRelay in job_config/script_runner.py, flower_job.py). + + Args: + fl_ctx: an FLContext to fire the event with. + dxo: the analytics data (e.g. from create_analytic_dxo) carried by the event. + """ + if self._analytics_fire_fed_event: + send_analytic_dxo( + self, + dxo=dxo, + fl_ctx=fl_ctx, + event_type=FED_ANALYTIC_EVENT_TYPE, + fire_fed_event=True, + ) + else: + send_analytic_dxo( + self, + dxo=dxo, + fl_ctx=fl_ctx, + event_type=ANALYTIC_EVENT_TYPE, + fire_fed_event=False, + ) + + @staticmethod + def _normalize_optional_str(value: Optional[str]) -> Optional[str]: + """Treats an empty/whitespace-only string as unset (None).""" + if isinstance(value, str) and not value.strip(): + return None + return value + + @staticmethod + def _wrong_mode_error(arg_name: str, value, valid_modes: str, execution_mode: str) -> ValueError: + """Builds the consistent 'arg only valid for ' rejection error.""" + return ValueError( + f"{arg_name} is only valid for execution_mode {valid_modes}, " + f"but got {arg_name}={value!r} with execution_mode '{execution_mode}'" + ) + + def _build_backend_context(self) -> ClientAPIBackendContext: + """Builds the frozen config snapshot handed to the backend at initialize() (finding 11).""" + return ClientAPIBackendContext( + executor=self, + execution_mode=self._execution_mode, + task_script_path=self._task_script_path, + task_script_args=self._task_script_args, + command=self._command, + launch_once=self._launch_once, + launch_timeout=self._launch_timeout, + shutdown_timeout=self._shutdown_timeout, + stop_grace_period=self._stop_grace_period, + heartbeat_interval=self._heartbeat_interval, + heartbeat_timeout=self._heartbeat_timeout, + task_wait_timeout=self._task_wait_timeout, + result_wait_timeout=self._result_wait_timeout, + params_exchange_format=self._params_exchange_format, + params_transfer_type=self._params_transfer_type, + server_expected_format=self._server_expected_format, + from_nvflare_converter_id=self._from_nvflare_converter_id, + to_nvflare_converter_id=self._to_nvflare_converter_id, + train_task_name=self._train_task_name, + evaluate_task_name=self._evaluate_task_name, + submit_model_task_name=self._submit_model_task_name, + train_with_evaluation=self._train_with_evaluation, + memory_gc_rounds=self._memory_gc_rounds, + cuda_empty_cache=self._cuda_empty_cache, + attach_timeout=self._attach_timeout, + allow_reconnect=self._allow_reconnect, + ) + + def _backend_registry(self) -> Dict[str, Callable[[], ClientAPIBackendSpec]]: + """Internal registry mapping each execution_mode to its backend factory. + + Backend PRs replace the corresponding factory below with one that returns a real + ClientAPIBackendSpec; the registry keys are the frozen mode names. + """ + return { + ExecutionMode.IN_PROCESS: self._create_in_process_backend, + ExecutionMode.EXTERNAL_PROCESS: self._create_external_process_backend, + ExecutionMode.ATTACH: self._create_attach_backend, + } + + def _create_backend(self) -> ClientAPIBackendSpec: + factory = self._backend_registry().get(self._execution_mode) + if factory is None: + # Unreachable via the public constructor (execution_mode is validated there); + # guards subclasses that override _backend_registry(). + raise ValueError( + f"no backend factory registered for execution_mode '{self._execution_mode}': " + f"registered modes are {list(self._backend_registry().keys())}" + ) + return factory() + + def _create_in_process_backend(self) -> ClientAPIBackendSpec: + # Implemented in a follow-up PR (plan: EX-3). + raise NotImplementedError("in_process execution mode is not yet implemented in this release") + + def _create_external_process_backend(self) -> ClientAPIBackendSpec: + # Implemented in a follow-up PR (plan: EP-4). + raise NotImplementedError("external_process execution mode is not yet implemented in this release") + + def _create_attach_backend(self) -> ClientAPIBackendSpec: + # Implemented in a follow-up PR (plan: AT-2). + raise NotImplementedError("attach execution mode is not yet implemented in this release") diff --git a/tests/unit_test/app_common/executors/client_api_executor_test.py b/tests/unit_test/app_common/executors/client_api_executor_test.py new file mode 100644 index 0000000000..b17023568b --- /dev/null +++ b/tests/unit_test/app_common/executors/client_api_executor_test.py @@ -0,0 +1,577 @@ +# Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Tests for the ClientAPIExecutor interface-freeze skeleton (plan: EX-2). + +Covers the constructor-validation matrix, the per-mode dispatch failure behavior +(NotImplementedError naming the follow-up PR + system_panic, no hang), the analytics-event +ownership hook, and the surface-freeze contract on the frozen constructor parameter list. +""" + +import inspect +from unittest.mock import Mock + +import pytest + +from nvflare.apis.analytix import ANALYTIC_EVENT_TYPE, AnalyticsDataType +from nvflare.apis.event_type import EventType +from nvflare.apis.executor import Executor +from nvflare.apis.fl_constant import EventScope, FLContextKey, ReservedKey, ReturnCode +from nvflare.apis.fl_context import FLContext +from nvflare.apis.fl_exception import UnsafeComponentError, UnsafeJobError +from nvflare.apis.shareable import Shareable, make_reply +from nvflare.apis.signal import Signal +from nvflare.apis.utils.analytix_utils import create_analytic_dxo +from nvflare.app_common.app_constant import AppConstants +from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext, ClientAPIBackendSpec +from nvflare.app_common.executors.client_api_executor import ( + ALL_EXECUTION_MODES, + FED_ANALYTIC_EVENT_TYPE, + ClientAPIExecutor, + ExecutionMode, +) +from nvflare.client.config import ExchangeFormat, TransferType + +# The frozen V1 constructor surface (design: "Configuration Surface" in +# docs/design/client_api_execution_modes.md, plus the load-bearing args added beyond that list - +# documented in the ClientAPIExecutor module docstring). Renaming, removing, or reordering any of +# these is a breaking change to the interface freeze consumed by the mode backends. +FROZEN_CONSTRUCTOR_PARAMS = [ + "execution_mode", + "command", + "task_script_path", + "task_script_args", + "launch_once", + "launch_timeout", + "shutdown_timeout", + "stop_grace_period", + "heartbeat_interval", + "heartbeat_timeout", + "task_wait_timeout", + "result_wait_timeout", + "params_exchange_format", + "params_transfer_type", + "server_expected_format", + "from_nvflare_converter_id", + "to_nvflare_converter_id", + "train_task_name", + "evaluate_task_name", + "submit_model_task_name", + "train_with_evaluation", + "memory_gc_rounds", + "cuda_empty_cache", + "attach_timeout", + "allow_reconnect", +] + +# Minimal valid constructor kwargs per mode. +MODE_KWARGS = { + ExecutionMode.IN_PROCESS: {"execution_mode": "in_process"}, + ExecutionMode.EXTERNAL_PROCESS: {"execution_mode": "external_process", "command": "python custom/train.py"}, + ExecutionMode.ATTACH: {"execution_mode": "attach"}, +} + + +def _make_fl_ctx(engine) -> FLContext: + fl_ctx = FLContext() + fl_ctx.put(key=ReservedKey.ENGINE, value=engine, private=True, sticky=False) + return fl_ctx + + +def _make_recording_engine(): + """Returns (engine, fired) where fired collects (event_type, EVENT_DATA-at-fire-time).""" + engine = Mock() + fired = [] + + def _record(event_type, fl_ctx): + fired.append((event_type, fl_ctx.get_prop(FLContextKey.EVENT_DATA))) + + engine.fire_event.side_effect = _record + return engine, fired + + +class _StubBackend(ClientAPIBackendSpec): + """A minimal working backend to verify executor-to-backend plumbing.""" + + def __init__(self): + self.calls = [] + self.result = make_reply(ReturnCode.OK) + self.context = None + + def initialize(self, context, fl_ctx): + self.context = context + self.calls.append("initialize") + + def execute(self, task_name, shareable, fl_ctx, abort_signal): + self.calls.append(("execute", task_name)) + return self.result + + def handle_event(self, event_type, fl_ctx): + self.calls.append(("handle_event", event_type)) + + def finalize(self, fl_ctx): + self.calls.append("finalize") + + +class _StubbedInProcessExecutor(ClientAPIExecutor): + """ClientAPIExecutor with a working in_process backend factory (as EX-3 will provide).""" + + def __init__(self, backend, **kwargs): + super().__init__(**kwargs) + self._stub_backend = backend + + def _create_in_process_backend(self): + return self._stub_backend + + +class TestConstructorValidation: + @pytest.mark.parametrize("kwargs", list(MODE_KWARGS.values()), ids=list(MODE_KWARGS.keys())) + def test_valid_minimal_construction(self, kwargs): + executor = ClientAPIExecutor(**kwargs) + assert isinstance(executor, Executor) + assert executor._execution_mode == kwargs["execution_mode"] + + def test_valid_attach_with_attach_args(self): + executor = ClientAPIExecutor(execution_mode="attach", attach_timeout=60.0, allow_reconnect=True) + assert executor._attach_timeout == 60.0 + assert executor._allow_reconnect is True + + def test_valid_full_surface(self): + executor = ClientAPIExecutor( + execution_mode="external_process", + command="torchrun --nproc_per_node=2 custom/train.py", + launch_once=False, + launch_timeout=120.0, + shutdown_timeout=60.0, + stop_grace_period=10.0, + heartbeat_interval=2.0, + heartbeat_timeout=20.0, + task_wait_timeout=300.0, + result_wait_timeout=600.0, + params_exchange_format=ExchangeFormat.PYTORCH, + params_transfer_type=TransferType.DIFF, + server_expected_format=ExchangeFormat.NUMPY, + from_nvflare_converter_id="from_converter", + to_nvflare_converter_id="to_converter", + train_task_name="my_train", + evaluate_task_name="my_eval", + submit_model_task_name="my_submit", + train_with_evaluation=True, + memory_gc_rounds=5, + cuda_empty_cache=True, + ) + assert executor._command == "torchrun --nproc_per_node=2 custom/train.py" + assert executor._launch_once is False + assert executor._params_exchange_format == ExchangeFormat.PYTORCH + assert executor._params_transfer_type == TransferType.DIFF + assert executor._train_task_name == "my_train" + assert executor._evaluate_task_name == "my_eval" + assert executor._submit_model_task_name == "my_submit" + assert executor._train_with_evaluation is True + assert executor._memory_gc_rounds == 5 + assert executor._cuda_empty_cache is True + + def test_task_name_and_memory_defaults(self): + # findings 9 + 10: rank-contract task names and memory-management knobs are frozen with the + # legacy executors' defaults, valid in every mode. + executor = ClientAPIExecutor(execution_mode="in_process") + assert executor._train_task_name == AppConstants.TASK_TRAIN + assert executor._evaluate_task_name == AppConstants.TASK_VALIDATION + assert executor._submit_model_task_name == AppConstants.TASK_SUBMIT_MODEL + assert executor._train_with_evaluation is False + assert executor._memory_gc_rounds == 0 + assert executor._cuda_empty_cache is False + + def test_in_process_accepts_task_script(self): + # finding 8: in_process names its script via task_script_path/args (command names the + # external_process trainer). + executor = ClientAPIExecutor( + execution_mode="in_process", task_script_path="custom/train.py", task_script_args="--epochs 3" + ) + assert executor._task_script_path == "custom/train.py" + assert executor._task_script_args == "--epochs 3" + assert executor._command is None + + @pytest.mark.parametrize("bad_mode", ["IN_PROCESS", "In_Process", "subprocess", "launch", "", None, 1]) + def test_unknown_execution_mode_rejected(self, bad_mode): + with pytest.raises(ValueError, match="invalid execution_mode"): + ClientAPIExecutor(execution_mode=bad_mode) + + def test_unknown_execution_mode_message_names_valid_modes(self): + with pytest.raises(ValueError) as exc_info: + ClientAPIExecutor(execution_mode="bogus") + for mode in ALL_EXECUTION_MODES: + assert mode in str(exc_info.value) + + @pytest.mark.parametrize("mode", ["in_process", "attach"]) + def test_command_rejected_for_non_external_process_modes(self, mode): + with pytest.raises(ValueError, match="command is only valid for execution_mode 'external_process'"): + ClientAPIExecutor(execution_mode=mode, command="python custom/train.py") + + @pytest.mark.parametrize("command", [None, ""]) + def test_external_process_requires_command(self, command): + with pytest.raises(ValueError, match="'external_process' requires a non-empty command"): + ClientAPIExecutor(execution_mode="external_process", command=command) + + @pytest.mark.parametrize( + "kwargs", + [ + {"execution_mode": "in_process"}, + {"execution_mode": "external_process", "command": "python custom/train.py"}, + ], + ids=["in_process", "external_process"], + ) + def test_attach_timeout_rejected_for_non_attach_modes(self, kwargs): + with pytest.raises(ValueError, match="attach_timeout is only valid for execution_mode 'attach'"): + ClientAPIExecutor(attach_timeout=30.0, **kwargs) + + @pytest.mark.parametrize( + "kwargs", + [ + {"execution_mode": "in_process"}, + {"execution_mode": "external_process", "command": "python custom/train.py"}, + ], + ids=["in_process", "external_process"], + ) + def test_allow_reconnect_rejected_for_non_attach_modes(self, kwargs): + with pytest.raises(ValueError, match="allow_reconnect is only valid for execution_mode 'attach'"): + ClientAPIExecutor(allow_reconnect=True, **kwargs) + + @pytest.mark.parametrize("mode", ["in_process", "attach"]) + @pytest.mark.parametrize("command", ["", " ", "\t"]) + def test_empty_command_treated_as_unset_for_non_external_modes(self, mode, command): + # finding 1: an empty/whitespace command is "unset", not a wrong-mode command, so it must + # not be rejected with the misleading "only valid for external_process" message. + executor = ClientAPIExecutor(execution_mode=mode, command=command) + assert executor._command is None + + @pytest.mark.parametrize("command", ["", " "]) + def test_external_process_rejects_empty_command(self, command): + # finding 1: the same normalization must keep external_process requiring a real command. + with pytest.raises(ValueError, match="'external_process' requires a non-empty command"): + ClientAPIExecutor(execution_mode="external_process", command=command) + + @pytest.mark.parametrize("mode", ["external_process", "attach"]) + def test_task_script_path_rejected_for_non_in_process_modes(self, mode): + # finding 8: task_script_path names the in_process script only. + kwargs = dict(MODE_KWARGS[mode]) + with pytest.raises(ValueError, match="task_script_path is only valid for execution_mode 'in_process'"): + ClientAPIExecutor(task_script_path="custom/train.py", **kwargs) + + @pytest.mark.parametrize("mode", ["external_process", "attach"]) + def test_task_script_args_rejected_for_non_in_process_modes(self, mode): + kwargs = dict(MODE_KWARGS[mode]) + with pytest.raises(ValueError, match="task_script_args is only valid for execution_mode 'in_process'"): + ClientAPIExecutor(task_script_args="--epochs 3", **kwargs) + + @pytest.mark.parametrize("mode", ["in_process", "attach"]) + def test_launch_once_non_default_rejected_for_non_external_modes(self, mode): + # finding 3: external_process-only knobs must be rejected (not silently ignored) when set + # to a non-default value in a mode that ignores them. + kwargs = dict(MODE_KWARGS[mode]) + with pytest.raises(ValueError, match="launch_once is only valid for execution_mode 'external_process'"): + ClientAPIExecutor(launch_once=False, **kwargs) + + @pytest.mark.parametrize("mode", ["in_process", "attach"]) + def test_launch_timeout_rejected_for_non_external_modes(self, mode): + kwargs = dict(MODE_KWARGS[mode]) + with pytest.raises(ValueError, match="launch_timeout is only valid for execution_mode 'external_process'"): + ClientAPIExecutor(launch_timeout=120.0, **kwargs) + + @pytest.mark.parametrize("mode", ["in_process", "attach"]) + def test_shutdown_timeout_rejected_for_non_external_modes(self, mode): + kwargs = dict(MODE_KWARGS[mode]) + with pytest.raises(ValueError, match="shutdown_timeout is only valid for execution_mode 'external_process'"): + ClientAPIExecutor(shutdown_timeout=60.0, **kwargs) + + @pytest.mark.parametrize("mode", ["in_process", "attach"]) + def test_stop_grace_period_non_default_rejected_for_non_external_modes(self, mode): + kwargs = dict(MODE_KWARGS[mode]) + with pytest.raises(ValueError, match="stop_grace_period is only valid for execution_mode 'external_process'"): + ClientAPIExecutor(stop_grace_period=10.0, **kwargs) + + @pytest.mark.parametrize("arg", ["heartbeat_interval", "heartbeat_timeout"]) + def test_heartbeat_non_default_rejected_for_in_process(self, arg): + # finding 3: there is no session heartbeat in_process, so a non-default heartbeat knob is + # dead there and must be rejected (valid for external_process/attach). + with pytest.raises(ValueError, match=f"{arg} is only valid for execution_mode 'external_process' or 'attach'"): + ClientAPIExecutor(execution_mode="in_process", **{arg: 99.0}) + + @pytest.mark.parametrize("mode", ["external_process", "attach"]) + def test_heartbeat_accepted_for_out_of_process_modes(self, mode): + kwargs = dict(MODE_KWARGS[mode]) + executor = ClientAPIExecutor(heartbeat_interval=2.0, heartbeat_timeout=20.0, **kwargs) + assert executor._heartbeat_interval == 2.0 + assert executor._heartbeat_timeout == 20.0 + + @pytest.mark.parametrize("mode", ["in_process", "attach"]) + def test_external_process_defaults_accepted_for_other_modes(self, mode): + # The frozen defaults for external_process-only knobs are indistinguishable from "not set" + # and must be accepted in every mode. + kwargs = dict(MODE_KWARGS[mode]) + ClientAPIExecutor(launch_once=True, stop_grace_period=30.0, **kwargs) + + @pytest.mark.parametrize("mode", ["in_process", "external_process"]) + def test_allow_reconnect_default_value_accepted_for_non_attach_modes(self, mode): + # allow_reconnect=False equals the frozen default and is indistinguishable from + # "not set", so it must not be rejected. + kwargs = dict(MODE_KWARGS[mode]) + ClientAPIExecutor(allow_reconnect=False, **kwargs) + + @pytest.mark.parametrize("mode", ["in_process", "external_process"]) + @pytest.mark.parametrize("falsy", [None, 0]) + def test_allow_reconnect_falsy_values_accepted_for_non_attach_modes(self, mode, falsy): + # finding 2: the wrong-mode check uses `if allow_reconnect` (truthy), not + # `is not False`, so falsy-but-not-False values (None, 0, numpy.bool_(False)) are treated + # as "not set" instead of misfiring. + kwargs = dict(MODE_KWARGS[mode]) + ClientAPIExecutor(allow_reconnect=falsy, **kwargs) + + +class TestDispatch: + """All three modes are skeleton-only in EX-2: resolving the backend at START_RUN must fail + the job cleanly (system_panic naming the mode), and execute() must reply with an error instead + of hanging.""" + + @pytest.mark.parametrize("mode", list(ALL_EXECUTION_MODES)) + def test_backend_factory_raises_not_implemented(self, mode): + # finding 7: user-facing message must not carry an internal plan id (EX-3/EP-4/AT-2); it + # names the mode and says "not yet implemented". + executor = ClientAPIExecutor(**MODE_KWARGS[mode]) + with pytest.raises(NotImplementedError, match="not yet implemented") as exc_info: + executor._create_backend() + message = str(exc_info.value) + assert mode in message + for plan_id in ("EX-3", "EP-4", "AT-2"): + assert plan_id not in message + + @pytest.mark.parametrize("mode", list(ALL_EXECUTION_MODES)) + def test_start_run_panics_naming_the_mode(self, mode): + # findings 6 + 7: the panic reason names the mode directly (not via secure_format_exception, + # so it is robust whether or not NVFLARE_SECURE_LOGGING is set) and carries no plan id. + executor = ClientAPIExecutor(**MODE_KWARGS[mode]) + engine, fired = _make_recording_engine() + fl_ctx = _make_fl_ctx(engine) + + executor.handle_event(EventType.START_RUN, fl_ctx) + + panics = [(event_type, data) for event_type, data in fired if event_type == EventType.FATAL_SYSTEM_ERROR] + assert len(panics) == 1, f"expected exactly one system_panic, got events: {fired}" + reason = panics[0][1] + assert mode in reason + for plan_id in ("EX-3", "EP-4", "AT-2"): + assert plan_id not in reason + + @pytest.mark.parametrize("mode", list(ALL_EXECUTION_MODES)) + def test_execute_without_backend_returns_error_reply_not_hang(self, mode): + executor = ClientAPIExecutor(**MODE_KWARGS[mode]) + engine, _ = _make_recording_engine() + fl_ctx = _make_fl_ctx(engine) + executor.handle_event(EventType.START_RUN, fl_ctx) # backend init fails -> panic + + reply = executor.execute("train", Shareable(), fl_ctx, Signal()) + + assert isinstance(reply, Shareable) + assert reply.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + + def test_end_run_without_backend_does_not_raise(self): + executor = ClientAPIExecutor(execution_mode="in_process") + engine, fired = _make_recording_engine() + fl_ctx = _make_fl_ctx(engine) + executor.handle_event(EventType.END_RUN, fl_ctx) + assert not any(event_type == EventType.FATAL_SYSTEM_ERROR for event_type, _ in fired) + + +class TestBackendPlumbing: + """With a working backend registered (as EX-3/EP-4/AT-2 will do), the executor drives the + ClientAPIBackendSpec lifecycle: initialize at START_RUN, execute per task, handle_event for + other events, finalize at END_RUN.""" + + def _make_started_executor(self): + backend = _StubBackend() + executor = _StubbedInProcessExecutor(backend, execution_mode="in_process") + engine, fired = _make_recording_engine() + fl_ctx = _make_fl_ctx(engine) + executor.handle_event(EventType.START_RUN, fl_ctx) + return executor, backend, fl_ctx, fired + + def test_start_run_initializes_backend_without_panic(self): + executor, backend, fl_ctx, fired = self._make_started_executor() + assert backend.calls == ["initialize"] + assert not any(event_type == EventType.FATAL_SYSTEM_ERROR for event_type, _ in fired) + + def test_execute_delegates_to_backend(self): + executor, backend, fl_ctx, _ = self._make_started_executor() + reply = executor.execute("train", Shareable(), fl_ctx, Signal()) + assert reply is backend.result + assert ("execute", "train") in backend.calls + + def test_other_events_forwarded_to_backend(self): + executor, backend, fl_ctx, _ = self._make_started_executor() + executor.handle_event(EventType.ABOUT_TO_END_RUN, fl_ctx) + assert ("handle_event", EventType.ABOUT_TO_END_RUN) in backend.calls + + def test_end_run_finalizes_and_clears_backend(self): + executor, backend, fl_ctx, _ = self._make_started_executor() + executor.handle_event(EventType.END_RUN, fl_ctx) + assert "finalize" in backend.calls + reply = executor.execute("train", Shareable(), fl_ctx, Signal()) + assert reply.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + + def test_backend_exception_in_execute_becomes_error_reply(self): + executor, backend, fl_ctx, _ = self._make_started_executor() + backend.execute = Mock(side_effect=RuntimeError("boom")) + reply = executor.execute("train", Shareable(), fl_ctx, Signal()) + assert reply.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + + def test_unsafe_job_error_propagates_out_of_execute(self): + # ClientRunner has dedicated UNSAFE_JOB handling for UnsafeJobError; the executor must + # let it propagate instead of masking it as EXECUTION_EXCEPTION. + executor, backend, fl_ctx, _ = self._make_started_executor() + backend.execute = Mock(side_effect=UnsafeJobError("unsafe")) + with pytest.raises(UnsafeJobError): + executor.execute("train", Shareable(), fl_ctx, Signal()) + + def test_unsafe_component_error_becomes_execution_exception(self): + # UnsafeComponentError has no dedicated ClientRunner handling, so it takes the generic + # error path (its existing behavior) rather than propagating. + executor, backend, fl_ctx, _ = self._make_started_executor() + backend.execute = Mock(side_effect=UnsafeComponentError("bad component")) + reply = executor.execute("train", Shareable(), fl_ctx, Signal()) + assert reply.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + + def test_backend_non_shareable_result_becomes_error_reply(self): + executor, backend, fl_ctx, _ = self._make_started_executor() + backend.result = {"not": "a shareable"} + reply = executor.execute("train", Shareable(), fl_ctx, Signal()) + assert reply.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + + def test_backend_receives_config_context(self): + # finding 11: initialize() receives a frozen ClientAPIBackendContext carrying the executor + # config and a back-reference to the executor (for fire_log_analytics/logging). + backend = _StubBackend() + executor = _StubbedInProcessExecutor( + backend, + execution_mode="in_process", + task_script_path="custom/train.py", + train_task_name="my_train", + memory_gc_rounds=7, + ) + engine, _ = _make_recording_engine() + executor.handle_event(EventType.START_RUN, _make_fl_ctx(engine)) + + ctx = backend.context + assert isinstance(ctx, ClientAPIBackendContext) + assert ctx.executor is executor + assert ctx.execution_mode == "in_process" + assert ctx.task_script_path == "custom/train.py" + assert ctx.train_task_name == "my_train" + assert ctx.memory_gc_rounds == 7 + + def test_backend_context_is_frozen(self): + backend = _StubBackend() + executor = _StubbedInProcessExecutor(backend, execution_mode="in_process") + executor.handle_event(EventType.START_RUN, _make_fl_ctx(_make_recording_engine()[0])) + with pytest.raises(Exception): + backend.context.execution_mode = "attach" + + def test_backend_spec_is_abstract(self): + with pytest.raises(TypeError): + ClientAPIBackendSpec() + + +class TestAnalyticsOwnership: + """fire_log_analytics is the executor-owned LOG-to-analytics conversion surface.""" + + def _fire(self, fire_fed_event: bool): + executor = ClientAPIExecutor(execution_mode="in_process") + executor._analytics_fire_fed_event = fire_fed_event + engine = Mock() + scopes = [] + engine.fire_event.side_effect = lambda event_type, ctx: scopes.append( + (event_type, ctx.get_prop(FLContextKey.EVENT_SCOPE)) + ) + fl_ctx = _make_fl_ctx(engine) + dxo = create_analytic_dxo(tag="loss", value=0.1, data_type=AnalyticsDataType.SCALAR) + executor.fire_log_analytics(fl_ctx, dxo) + return scopes + + def test_default_path_fires_local_analytics_event(self): + # Default: local un-prefixed event; ConvertToFedEvent (added by BaseFedJob) is + # responsible for re-firing it as "fed.analytix_log_stats". + scopes = self._fire(fire_fed_event=False) + assert scopes == [(ANALYTIC_EVENT_TYPE, EventScope.LOCAL)] + + def test_fed_path_fires_federation_scoped_event(self): + # finding 4: the fed path must fire the already-"fed."-prefixed event name so it lands on + # the same server-side event as MetricRelay (job_config/script_runner.py) and flower_job.py + # ("fed.analytix_log_stats"); firing the un-prefixed name federation-scoped would miss every + # consumer listening on "fed.analytix_log_stats". + assert FED_ANALYTIC_EVENT_TYPE == "fed.analytix_log_stats" + scopes = self._fire(fire_fed_event=True) + assert scopes == [(FED_ANALYTIC_EVENT_TYPE, EventScope.FEDERATION)] + + +class TestSurfaceFreeze: + """Interface freeze #2: accidental renames/reorders of the constructor surface fail CI.""" + + def test_constructor_parameter_names_match_frozen_list(self): + params = list(inspect.signature(ClientAPIExecutor.__init__).parameters) + assert params[0] == "self" + assert params[1:] == FROZEN_CONSTRUCTOR_PARAMS + + def test_execution_mode_is_required(self): + sig = inspect.signature(ClientAPIExecutor.__init__) + assert sig.parameters["execution_mode"].default is inspect.Parameter.empty + + def test_frozen_defaults(self): + # launch_once/heartbeat_interval/heartbeat_timeout/allow_reconnect defaults are + # normative from the design's Configuration Surface; the rest are frozen by EX-2. + sig = inspect.signature(ClientAPIExecutor.__init__) + expected_defaults = { + "command": None, + "task_script_path": None, + "task_script_args": "", + "launch_once": True, + "launch_timeout": None, + "shutdown_timeout": None, + "stop_grace_period": 30.0, + "heartbeat_interval": 5.0, + "heartbeat_timeout": 30.0, + "task_wait_timeout": None, + "result_wait_timeout": None, + "params_exchange_format": ExchangeFormat.NUMPY, + "params_transfer_type": TransferType.FULL, + "server_expected_format": ExchangeFormat.NUMPY, + "from_nvflare_converter_id": None, + "to_nvflare_converter_id": None, + "train_task_name": AppConstants.TASK_TRAIN, + "evaluate_task_name": AppConstants.TASK_VALIDATION, + "submit_model_task_name": AppConstants.TASK_SUBMIT_MODEL, + "train_with_evaluation": False, + "memory_gc_rounds": 0, + "cuda_empty_cache": False, + "attach_timeout": None, + "allow_reconnect": False, + } + actual_defaults = {name: p.default for name, p in sig.parameters.items() if name != "self"} + actual_defaults.pop("execution_mode") + assert actual_defaults == expected_defaults + + def test_module_path_is_pinned(self): + # The design's example job config pins this exact path. + assert ClientAPIExecutor.__module__ == "nvflare.app_common.executors.client_api_executor" + + def test_execution_mode_values_are_frozen(self): + assert ALL_EXECUTION_MODES == ("in_process", "external_process", "attach") From 183430f84649366563ab85a3f26bdaff7f7260af Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Thu, 2 Jul 2026 13:21:46 -0700 Subject: [PATCH 02/12] EX-2: drop param converters from the frozen surface (FLARE-2698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FLARE-2698 bullet 2 removes params converters in favor of filters. Since EX-2 is an interface freeze, freezing args we intend to delete would make their later removal a breaking change — so exclude them now. Removed from the ClientAPIExecutor constructor: params_exchange_format, params_transfer_type, server_expected_format, from_nvflare_converter_id, to_nvflare_converter_id (and from ClientAPIBackendContext). Param conversion between the framework-agnostic aggregation representation (numpy) and the framework-native training representation moves to send/receive filters at the client edge (tracked as EX-5 / the converter->filter migration); the executor and Cell layers pass through. Transfer type FULL/DIFF is not a converter and stays a Client API (model_registry) concern. Surface-freeze test updated to the trimmed arg list. 83 tests pass. --- .../executors/client_api/backend_spec.py | 12 +++---- .../executors/client_api_executor.py | 32 ++++--------------- .../executors/client_api_executor_test.py | 18 ----------- 3 files changed, 13 insertions(+), 49 deletions(-) diff --git a/nvflare/app_common/executors/client_api/backend_spec.py b/nvflare/app_common/executors/client_api/backend_spec.py index c40801ea96..5893ceea8d 100644 --- a/nvflare/app_common/executors/client_api/backend_spec.py +++ b/nvflare/app_common/executors/client_api/backend_spec.py @@ -76,12 +76,12 @@ class ClientAPIBackendContext: heartbeat_timeout: float = 30.0 task_wait_timeout: Optional[float] = None result_wait_timeout: Optional[float] = None - # data / params - params_exchange_format: str = "numpy" - params_transfer_type: str = "FULL" - server_expected_format: str = "numpy" - from_nvflare_converter_id: Optional[str] = None - to_nvflare_converter_id: Optional[str] = None + # NOTE: params_exchange_format / params_transfer_type / server_expected_format and the + # from/to_nvflare_converter ids are intentionally NOT here. Per FLARE-2698, param + # conversion between the framework-agnostic aggregation representation (numpy) and the + # framework-native training representation moves out of the executor to send/receive + # filters at the client edge; the Client API boundary is pass-through. Transfer type + # (FULL/DIFF) stays a Client API concern (model_registry), decided separately. # task-name / rank contract (all modes) train_task_name: str = "train" evaluate_task_name: str = "validate" diff --git a/nvflare/app_common/executors/client_api_executor.py b/nvflare/app_common/executors/client_api_executor.py index b156737624..db58743e05 100644 --- a/nvflare/app_common/executors/client_api_executor.py +++ b/nvflare/app_common/executors/client_api_executor.py @@ -35,6 +35,13 @@ ``train_with_evaluation`` - power the rank-contract APIs flare.is_train()/is_evaluate()/ is_submit_model(). - ``memory_gc_rounds`` / ``cuda_empty_cache`` - periodic GC / CUDA cache management + +Deliberately excluded (per FLARE-2698): ``params_exchange_format`` / ``params_transfer_type`` / +``server_expected_format`` / ``from_nvflare_converter_id`` / ``to_nvflare_converter_id``. Param +conversion moves from executor-owned ParamsConverters to send/receive filters at the client +edge (the intermediate layers pass through), so these are not frozen into this surface. The +transfer type (FULL/DIFF) remains a Client API concern (model_registry) and is decided +separately from the converter removal. (public on ScriptRunner and both legacy executors). """ @@ -53,7 +60,6 @@ from nvflare.app_common.app_constant import AppConstants from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext, ClientAPIBackendSpec from nvflare.app_common.widgets.convert_to_fed_event import FED_EVENT_PREFIX -from nvflare.client.config import ExchangeFormat, TransferType from nvflare.security.logging import secure_format_exception, secure_format_traceback @@ -106,11 +112,6 @@ def __init__( heartbeat_timeout: float = _DEFAULT_HEARTBEAT_TIMEOUT, task_wait_timeout: Optional[float] = None, result_wait_timeout: Optional[float] = None, - params_exchange_format: str = ExchangeFormat.NUMPY, - params_transfer_type: str = TransferType.FULL, - server_expected_format: str = ExchangeFormat.NUMPY, - from_nvflare_converter_id: Optional[str] = None, - to_nvflare_converter_id: Optional[str] = None, train_task_name: str = AppConstants.TASK_TRAIN, evaluate_task_name: str = AppConstants.TASK_VALIDATION, submit_model_task_name: str = AppConstants.TASK_SUBMIT_MODEL, @@ -156,15 +157,6 @@ def __init__( result_wait_timeout (Optional[float]): Control-side bound for retrieving the task result. Payload transfer completion is governed by the shared transfer layer, not by this value. None means no timeout. - params_exchange_format (str): Format to exchange parameters with the training - script, e.g. "pytorch", "numpy", "raw" (see ExchangeFormat). - params_transfer_type (str): "FULL" (whole model) or "DIFF" (difference only). - server_expected_format (str): Format to exchange parameters between server and - client (see ExchangeFormat). - from_nvflare_converter_id (Optional[str]): Component id of the ParamsConverter - applied to task data sent from the NVFlare controller side to the trainer side. - to_nvflare_converter_id (Optional[str]): Component id of the ParamsConverter - applied to results sent from the trainer side to the NVFlare controller side. train_task_name (str): Task name treated as "train" by flare.is_train() (rank contract). Defaults to AppConstants.TASK_TRAIN. evaluate_task_name (str): Task name treated as "evaluate" by flare.is_evaluate(). @@ -259,11 +251,6 @@ def __init__( self._heartbeat_timeout = heartbeat_timeout self._task_wait_timeout = task_wait_timeout self._result_wait_timeout = result_wait_timeout - self._params_exchange_format = params_exchange_format - self._params_transfer_type = params_transfer_type - self._server_expected_format = server_expected_format - self._from_nvflare_converter_id = from_nvflare_converter_id - self._to_nvflare_converter_id = to_nvflare_converter_id self._train_task_name = train_task_name self._evaluate_task_name = evaluate_task_name self._submit_model_task_name = submit_model_task_name @@ -423,11 +410,6 @@ def _build_backend_context(self) -> ClientAPIBackendContext: heartbeat_timeout=self._heartbeat_timeout, task_wait_timeout=self._task_wait_timeout, result_wait_timeout=self._result_wait_timeout, - params_exchange_format=self._params_exchange_format, - params_transfer_type=self._params_transfer_type, - server_expected_format=self._server_expected_format, - from_nvflare_converter_id=self._from_nvflare_converter_id, - to_nvflare_converter_id=self._to_nvflare_converter_id, train_task_name=self._train_task_name, evaluate_task_name=self._evaluate_task_name, submit_model_task_name=self._submit_model_task_name, diff --git a/tests/unit_test/app_common/executors/client_api_executor_test.py b/tests/unit_test/app_common/executors/client_api_executor_test.py index b17023568b..a8f710c6c4 100644 --- a/tests/unit_test/app_common/executors/client_api_executor_test.py +++ b/tests/unit_test/app_common/executors/client_api_executor_test.py @@ -41,7 +41,6 @@ ClientAPIExecutor, ExecutionMode, ) -from nvflare.client.config import ExchangeFormat, TransferType # The frozen V1 constructor surface (design: "Configuration Surface" in # docs/design/client_api_execution_modes.md, plus the load-bearing args added beyond that list - @@ -60,11 +59,6 @@ "heartbeat_timeout", "task_wait_timeout", "result_wait_timeout", - "params_exchange_format", - "params_transfer_type", - "server_expected_format", - "from_nvflare_converter_id", - "to_nvflare_converter_id", "train_task_name", "evaluate_task_name", "submit_model_task_name", @@ -159,11 +153,6 @@ def test_valid_full_surface(self): heartbeat_timeout=20.0, task_wait_timeout=300.0, result_wait_timeout=600.0, - params_exchange_format=ExchangeFormat.PYTORCH, - params_transfer_type=TransferType.DIFF, - server_expected_format=ExchangeFormat.NUMPY, - from_nvflare_converter_id="from_converter", - to_nvflare_converter_id="to_converter", train_task_name="my_train", evaluate_task_name="my_eval", submit_model_task_name="my_submit", @@ -173,8 +162,6 @@ def test_valid_full_surface(self): ) assert executor._command == "torchrun --nproc_per_node=2 custom/train.py" assert executor._launch_once is False - assert executor._params_exchange_format == ExchangeFormat.PYTORCH - assert executor._params_transfer_type == TransferType.DIFF assert executor._train_task_name == "my_train" assert executor._evaluate_task_name == "my_eval" assert executor._submit_model_task_name == "my_submit" @@ -551,11 +538,6 @@ def test_frozen_defaults(self): "heartbeat_timeout": 30.0, "task_wait_timeout": None, "result_wait_timeout": None, - "params_exchange_format": ExchangeFormat.NUMPY, - "params_transfer_type": TransferType.FULL, - "server_expected_format": ExchangeFormat.NUMPY, - "from_nvflare_converter_id": None, - "to_nvflare_converter_id": None, "train_task_name": AppConstants.TASK_TRAIN, "evaluate_task_name": AppConstants.TASK_VALIDATION, "submit_model_task_name": AppConstants.TASK_SUBMIT_MODEL, From e3c10628fafbd97689acc26479f72185f9e0e418 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Tue, 7 Jul 2026 16:55:54 -0700 Subject: [PATCH 03/12] Add in_process backend for ClientAPIExecutor (EX-3) Implements the first real backend behind the frozen ClientAPIBackendSpec, porting InProcessClientAPIExecutor's DataBus machinery with the behavior-parity bar "nothing user-visible": trainer script on a thread in the CJ process, tasks over TOPIC_GLOBAL_RESULT, results over TOPIC_LOCAL_RESULT, API discovery via CLIENT_API_KEY. Design-mandated deviations from the legacy executor: - No ParamsConverters / exchange-format knobs (FLARE-2698): the boundary is pass-through (ExchangeFormat.RAW, TransferType.FULL; DIFF returns with the model_registry transfer-type decision). - LOG data routes through the executor-owned fire_log_analytics() (single analytics-event ownership point). - initialize() self-unwinds on failure; finalize() is idempotent and detaches ALL of this job's DataBus state -- including the InProcessClientAPI's own subscriptions via a new close() method (the singleton bus otherwise keeps a dead job's API subscribed, pinning each later job's global model). - execute() is bounded by result_wait_timeout (monotonic clock: a wall-clock step must not fire a spurious abort). After the in-process trainer aborts (script failure, timeout, STOP -- its thread is never relaunched), later tasks fail fast at entry with EXECUTION_EXCEPTION and the recorded first cause; an instant TASK_ABORTED would be misleading (task never delivered, abort_signal never triggered). Stale results from a timed-out task are dropped at the next task's entry. The executor's in_process factory now returns the real backend; the skeleton dispatch tests keep external_process/attach pinned as clean-failure modes. 18 backend tests drive the real DataBus round trip: multi-round execute, abort mid-task, post-timeout fail-fast, bounded wait, unwind on early/late init failure, idempotent finalize (exactly one STOP), successor CLIENT_API_KEY guard, dead-API subscription detach, LOG-to-analytics routing, RAW/FULL meta. --- .../client_api/in_process_backend.py | 311 ++++++++++++++ .../executors/client_api_executor.py | 7 +- nvflare/client/in_process/api.py | 13 + .../executors/client_api_executor_test.py | 20 +- .../executors/in_process_backend_test.py | 401 ++++++++++++++++++ 5 files changed, 746 insertions(+), 6 deletions(-) create mode 100644 nvflare/app_common/executors/client_api/in_process_backend.py create mode 100644 tests/unit_test/app_common/executors/in_process_backend_test.py diff --git a/nvflare/app_common/executors/client_api/in_process_backend.py b/nvflare/app_common/executors/client_api/in_process_backend.py new file mode 100644 index 0000000000..ac016cecc7 --- /dev/null +++ b/nvflare/app_common/executors/client_api/in_process_backend.py @@ -0,0 +1,311 @@ +# Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""in_process backend for ClientAPIExecutor (plan: EX-3). + +Ports InProcessClientAPIExecutor's DataBus machinery behind the frozen +ClientAPIBackendSpec surface, with the behavior-parity bar "nothing user-visible": +the trainer script still runs on a thread inside the CJ process, finds its +InProcessClientAPI via the DataBus CLIENT_API_KEY entry, receives tasks over +TOPIC_GLOBAL_RESULT, and returns results over TOPIC_LOCAL_RESULT. + +Differences from the legacy executor, all mandated by the design +(docs/design/client_api_execution_modes.md) and the backend contract: + +- No ParamsConverters and no exchange-format/transfer-type knobs (FLARE-2698): + the Client API boundary passes params through unconverted (ExchangeFormat.RAW) + and V1 sends full params (TransferType.FULL); DIFF support returns with the + model_registry transfer-type decision, and format conversion moves to + send/receive filters at the client edge. +- LOG data is converted to analytics events through the executor-owned + fire_log_analytics() (single analytics-event ownership point), not a direct + send_analytic_dxo call. +- initialize() self-unwinds on failure and finalize() is idempotent and + unsubscribes this backend's DataBus callbacks: the DataBus is a process + singleton, so leaked subscriptions would survive into later jobs run in the + same process (e.g. the simulator). +""" + +import threading +import time +from typing import Optional + +from nvflare.apis.fl_constant import FLContextKey, FLMetaKey, ReturnCode +from nvflare.apis.fl_context import FLContext +from nvflare.apis.shareable import Shareable, make_reply +from nvflare.apis.signal import Signal +from nvflare.apis.utils.analytix_utils import create_analytic_dxo +from nvflare.apis.workspace import Workspace +from nvflare.app_common.app_constant import AppConstants +from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext, ClientAPIBackendSpec +from nvflare.app_common.executors.task_script_runner import TaskScriptRunner +from nvflare.client.api_spec import CLIENT_API_KEY +from nvflare.client.config import ConfigKey, ExchangeFormat, TransferType +from nvflare.client.in_process.api import ( + TOPIC_ABORT, + TOPIC_GLOBAL_RESULT, + TOPIC_LOCAL_RESULT, + TOPIC_LOG_DATA, + TOPIC_STOP, + InProcessClientAPI, +) +from nvflare.fuel.data_event.data_bus import DataBus +from nvflare.fuel.data_event.event_manager import EventManager +from nvflare.fuel.utils.log_utils import get_obj_logger +from nvflare.security.logging import secure_format_traceback + +# Poll cadence for the result-wait loop and the trainer-side receive() checks. +# The legacy executor exposed this as result_pull_interval (default 0.5); the frozen +# surface deliberately drops the knob, so the default becomes the behavior. +_RESULT_POLL_INTERVAL = 0.5 + + +class InProcessBackend(ClientAPIBackendSpec): + """Runs the trainer script on a thread in the CJ process, bridged over DataBus.""" + + def __init__(self): + super().__init__() + # the spec is a plain ABC: fl_ctx-aware logging goes through the executor back-reference + # (context.executor.log_*); this logger covers callback paths that have no fl_ctx + self.logger = get_obj_logger(self) + self._context: Optional[ClientAPIBackendContext] = None + self._engine = None + self._data_bus: Optional[DataBus] = None + self._event_manager: Optional[EventManager] = None + self._client_api: Optional[InProcessClientAPI] = None + self._task_fn_thread: Optional[threading.Thread] = None + self._local_result: Optional[Shareable] = None + self._abort = False + self._abort_reason: Optional[str] = None + self._finalized = False + self._subscribed = False + + def initialize(self, context: ClientAPIBackendContext, fl_ctx: FLContext) -> None: + self._context = context + + task_script_path = context.task_script_path + if not task_script_path or not task_script_path.endswith(".py"): + raise ValueError(f"invalid task_script_path '{task_script_path}': in_process mode requires a .py script") + + try: + self._engine = fl_ctx.get_engine() + + self._data_bus = DataBus() + self._event_manager = EventManager(self._data_bus) + self._data_bus.subscribe([TOPIC_LOCAL_RESULT], self._local_result_callback) + self._data_bus.subscribe([TOPIC_LOG_DATA], self._log_result_callback) + self._data_bus.subscribe([TOPIC_ABORT, TOPIC_STOP], self._to_abort_callback) + self._subscribed = True + + workspace: Workspace = fl_ctx.get_prop(FLContextKey.WORKSPACE_OBJECT) + job_id = fl_ctx.get_prop(FLContextKey.CURRENT_JOB_ID) + custom_dir = workspace.get_app_custom_dir(job_id) + task_fn_wrapper = TaskScriptRunner( + custom_dir=custom_dir, script_path=task_script_path, script_args=context.task_script_args + ) + + meta = self._prepare_task_meta(fl_ctx, None) + self._client_api = InProcessClientAPI(task_metadata=meta, result_check_interval=_RESULT_POLL_INTERVAL) + self._client_api.init() + if context.memory_gc_rounds > 0: + self._client_api.configure_memory_management( + gc_rounds=context.memory_gc_rounds, cuda_empty_cache=context.cuda_empty_cache + ) + # this is how the trainer script's flare.init() finds the API instance + self._data_bus.put_data(CLIENT_API_KEY, self._client_api) + + # non-daemon, matching the legacy executor: process exit waits for the trainer, + # and finalize() joins it after TOPIC_STOP + self._task_fn_thread = threading.Thread(target=task_fn_wrapper.run, name="client_api_in_process_trainer") + self._task_fn_thread.start() + except Exception: + # contract (backend_spec): initialize() self-unwinds its partial setup on failure; + # the executor does not call finalize() on a half-initialized backend. + self._unwind() + raise + + def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort_signal: Signal) -> Shareable: + self._context.executor.log_info(fl_ctx, f"execute for task ({task_name})") + + if abort_signal.triggered: + self._event_manager.fire_event(TOPIC_ABORT, f"'{task_name}' is aborted, abort_signal_triggered") + return make_reply(ReturnCode.TASK_ABORTED) + + if self._abort: + # An in-process trainer that aborted (script failure, prior timeout, STOP) is gone + # for good -- the thread is never relaunched. Fail fast with an accurate return code: + # TASK_ABORTED here would be misleading (this task was never delivered and the + # abort_signal never triggered). The legacy executor could not reach this state in a + # healthy job (it had no result-wait bound), so this is new, documented behavior. + self._context.executor.log_error( + fl_ctx, + f"in-process trainer is no longer available (reason: {self._abort_reason}); " + f"failing task '{task_name}'", + ) + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + + # Drop any stale result from a previously timed-out task: a result published + # concurrently with that task's timeout decision must not satisfy this task's wait. + # No further correlation is needed: execute() runs one task at a time, and any + # trainer abort latches _abort above, so at most one such straggler can exist. + self._local_result = None + + try: + # kept from the legacy executor: some task scripts read this ad-hoc prop + fl_ctx.set_prop("abort_signal", abort_signal) + + meta = self._prepare_task_meta(fl_ctx, task_name) + self._client_api.set_meta(meta) + + shareable.set_header(FLMetaKey.JOB_ID, fl_ctx.get_job_id()) + shareable.set_header(FLMetaKey.SITE_NAME, fl_ctx.get_identity_name()) + + self._context.executor.log_info(fl_ctx, "sending task data to in-process trainer") + self._event_manager.fire_event(TOPIC_GLOBAL_RESULT, shareable) + + result_wait_timeout = self._context.result_wait_timeout + # monotonic: a wall-clock step (NTP, VM resume) must not fire a spurious timeout, + # which would kill the trainer for the rest of the job + wait_start = time.monotonic() + self._context.executor.log_info(fl_ctx, "waiting for result from in-process trainer") + while True: + if abort_signal.triggered or self._abort: + # notify the trainer that the task is aborted + self._event_manager.fire_event(TOPIC_ABORT, f"'{task_name}' is aborted, abort_signal_triggered") + return make_reply(ReturnCode.TASK_ABORTED) + + if self._local_result is not None: + result = self._local_result + self._local_result = None + + if not isinstance(result, Shareable): + self._context.executor.log_error( + fl_ctx, f"bad task result from trainer: expect Shareable but got {type(result)}" + ) + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + + current_round = shareable.get_header(AppConstants.CURRENT_ROUND) + if current_round is not None: + result.set_header(AppConstants.CURRENT_ROUND, current_round) + return result + + if result_wait_timeout is not None and time.monotonic() - wait_start > result_wait_timeout: + # the contract forbids waiting unbounded past the configured bound: tell the + # trainer to stop this task and fail the round + self._event_manager.fire_event( + TOPIC_ABORT, f"'{task_name}' timed out after {result_wait_timeout}s waiting for result" + ) + self._context.executor.log_error( + fl_ctx, f"timed out after {result_wait_timeout}s waiting for '{task_name}' result" + ) + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + + time.sleep(_RESULT_POLL_INTERVAL) + except Exception: + self._context.executor.log_error(fl_ctx, secure_format_traceback()) + self._event_manager.fire_event(TOPIC_ABORT, f"'{task_name}' failed: {secure_format_traceback()}") + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + + def handle_event(self, event_type: str, fl_ctx: FLContext) -> None: + # no per-event behavior for in_process (START_RUN/END_RUN are handled by the + # executor via initialize/finalize); contract: must not raise + pass + + def finalize(self, fl_ctx: FLContext) -> None: + # contract: idempotent and must not raise + if self._finalized: + return + self._finalized = True + # separate try blocks: a failure publishing the stop event must not skip the join + try: + if self._event_manager is not None: + self._event_manager.fire_event(TOPIC_STOP, "END_RUN received") + except Exception: + self.logger.error(secure_format_traceback()) + try: + thread = self._task_fn_thread + if thread is not None and thread.is_alive(): + thread.join() + except Exception: + self.logger.error(secure_format_traceback()) + finally: + self._unwind() + + def _unwind(self) -> None: + """Releases DataBus state so nothing leaks into later jobs (DataBus is a process singleton).""" + try: + if self._data_bus is not None: + if self._subscribed: + self._data_bus.unsubscribe(TOPIC_LOCAL_RESULT, self._local_result_callback) + self._data_bus.unsubscribe(TOPIC_LOG_DATA, self._log_result_callback) + self._data_bus.unsubscribe(TOPIC_ABORT, self._to_abort_callback) + self._data_bus.unsubscribe(TOPIC_STOP, self._to_abort_callback) + self._subscribed = False + if self._client_api is not None: + # detach the API's own subscriptions too: the singleton bus would otherwise + # keep the dead instance subscribed to TOPIC_GLOBAL_RESULT, pinning each + # later job's global model via the dead object + self._client_api.close() + if self._data_bus.get_data(CLIENT_API_KEY) is self._client_api: + # only clear our own entry: a later backend may have installed its API + self._data_bus.put_data(CLIENT_API_KEY, None) + except Exception: + self.logger.error(secure_format_traceback()) + self._client_api = None + + def _prepare_task_meta(self, fl_ctx: FLContext, task_name: Optional[str]) -> dict: + context = self._context + return { + FLMetaKey.SITE_NAME: fl_ctx.get_identity_name(), + FLMetaKey.JOB_ID: fl_ctx.get_job_id(), + ConfigKey.TASK_NAME: task_name, + ConfigKey.TASK_EXCHANGE: { + ConfigKey.TRAIN_WITH_EVAL: context.train_with_evaluation, + # FLARE-2698: the Client API boundary passes params through unconverted; format + # conversion happens in send/receive filters at the client edge, and DIFF returns + # with the model_registry transfer-type decision + ConfigKey.EXCHANGE_FORMAT: ExchangeFormat.RAW, + ConfigKey.TRANSFER_TYPE: TransferType.FULL, + ConfigKey.TRAIN_TASK_NAME: context.train_task_name, + ConfigKey.EVAL_TASK_NAME: context.evaluate_task_name, + ConfigKey.SUBMIT_MODEL_TASK_NAME: context.submit_model_task_name, + }, + } + + def _local_result_callback(self, topic, data, databus): + if not isinstance(data, Shareable): + # do not raise into the trainer's send path; record the bad result and let the + # execute() loop reply EXECUTION_EXCEPTION (a raise here would surface in the + # trainer thread, not the CJ) + self.logger.error(f"bad task result from trainer: expect Shareable but got {type(data)}") + self._local_result = data + + def _log_result_callback(self, topic, data, databus): + result = data + if result and not isinstance(result, dict): + raise ValueError(f"invalid result format, expecting Dict, but get {type(result)}") + + if "key" in result: + result["tag"] = result.pop("key") + dxo = create_analytic_dxo(**result) + + # single analytics-event ownership point: the executor decides local vs fed fire path + with self._engine.new_context() as fl_ctx: + self._context.executor.fire_log_analytics(fl_ctx, dxo) + + def _to_abort_callback(self, topic, data, databus): + self._abort = True + if self._abort_reason is None: + # keep the FIRST cause: later echoes (e.g. our own fail-fast fires) must not mask it + self._abort_reason = f"{topic}: {data}" diff --git a/nvflare/app_common/executors/client_api_executor.py b/nvflare/app_common/executors/client_api_executor.py index db58743e05..df31b4ea30 100644 --- a/nvflare/app_common/executors/client_api_executor.py +++ b/nvflare/app_common/executors/client_api_executor.py @@ -444,8 +444,11 @@ def _create_backend(self) -> ClientAPIBackendSpec: return factory() def _create_in_process_backend(self) -> ClientAPIBackendSpec: - # Implemented in a follow-up PR (plan: EX-3). - raise NotImplementedError("in_process execution mode is not yet implemented in this release") + # Deferred import: the backend pulls in DataBus/TaskScriptRunner machinery that the + # other modes never need; the skeleton stays import-light. + from nvflare.app_common.executors.client_api.in_process_backend import InProcessBackend + + return InProcessBackend() def _create_external_process_backend(self) -> ClientAPIBackendSpec: # Implemented in a follow-up PR (plan: EP-4). diff --git a/nvflare/client/in_process/api.py b/nvflare/client/in_process/api.py index 56a7a4af85..dccef79827 100644 --- a/nvflare/client/in_process/api.py +++ b/nvflare/client/in_process/api.py @@ -280,3 +280,16 @@ def shutdown(self): self.stop = True self.event_manager.fire_event(TOPIC_STOP) self.stop_reason = "API shutdown called." + + def close(self): + """Unsubscribes this API instance's DataBus callbacks. + + The DataBus is a process singleton: without this, a finished job's API instance + stays subscribed to TOPIC_GLOBAL_RESULT for the process lifetime, so every later + job's task publish also lands on the dead instance and pins its latest global + model in memory. Called by the owning executor/backend at teardown; safe to call + more than once (unsubscribe of an absent callback is a no-op). + """ + self.data_bus.unsubscribe(TOPIC_GLOBAL_RESULT, self.__receive_callback) + self.data_bus.unsubscribe(TOPIC_ABORT, self.__ask_to_abort) + self.data_bus.unsubscribe(TOPIC_STOP, self.__ask_to_abort) diff --git a/tests/unit_test/app_common/executors/client_api_executor_test.py b/tests/unit_test/app_common/executors/client_api_executor_test.py index a8f710c6c4..cd113c9283 100644 --- a/tests/unit_test/app_common/executors/client_api_executor_test.py +++ b/tests/unit_test/app_common/executors/client_api_executor_test.py @@ -327,11 +327,21 @@ def test_allow_reconnect_falsy_values_accepted_for_non_attach_modes(self, mode, class TestDispatch: - """All three modes are skeleton-only in EX-2: resolving the backend at START_RUN must fail - the job cleanly (system_panic naming the mode), and execute() must reply with an error instead - of hanging.""" + """in_process now resolves a real backend (EX-3); external_process/attach remain + skeleton-only: resolving those backends at START_RUN must fail the job cleanly + (system_panic naming the mode), and execute() must reply with an error instead of hanging. + in_process START_RUN without a valid task_script_path fails the same clean way, so the + all-modes panic tests below still hold.""" - @pytest.mark.parametrize("mode", list(ALL_EXECUTION_MODES)) + NOT_IMPLEMENTED_MODES = [ExecutionMode.EXTERNAL_PROCESS, ExecutionMode.ATTACH] + + def test_in_process_factory_returns_real_backend(self): + from nvflare.app_common.executors.client_api.in_process_backend import InProcessBackend + + executor = ClientAPIExecutor(**MODE_KWARGS[ExecutionMode.IN_PROCESS]) + assert isinstance(executor._create_backend(), InProcessBackend) + + @pytest.mark.parametrize("mode", NOT_IMPLEMENTED_MODES) def test_backend_factory_raises_not_implemented(self, mode): # finding 7: user-facing message must not carry an internal plan id (EX-3/EP-4/AT-2); it # names the mode and says "not yet implemented". @@ -347,6 +357,8 @@ def test_backend_factory_raises_not_implemented(self, mode): def test_start_run_panics_naming_the_mode(self, mode): # findings 6 + 7: the panic reason names the mode directly (not via secure_format_exception, # so it is robust whether or not NVFLARE_SECURE_LOGGING is set) and carries no plan id. + # For in_process the failure is now the missing task_script_path (backend initialize + # raises), not a NotImplementedError factory - the clean-failure contract is the same. executor = ClientAPIExecutor(**MODE_KWARGS[mode]) engine, fired = _make_recording_engine() fl_ctx = _make_fl_ctx(engine) diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py new file mode 100644 index 0000000000..42063b53d5 --- /dev/null +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -0,0 +1,401 @@ +# Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Tests for the in_process backend of ClientAPIExecutor (plan: EX-3). + +Drives the real DataBus round-trip the design defines for in_process: the backend fires the +task on TOPIC_GLOBAL_RESULT, a fake trainer replies on TOPIC_LOCAL_RESULT, and execute() +returns the result. Also covers the backend-contract obligations the legacy executor did not +have: initialize() self-unwinding, finalize() idempotency + DataBus cleanup (the DataBus is a +process singleton, so leaks would cross into later jobs in the same process), bounded result +wait, and LOG routing through the executor-owned fire_log_analytics(). +""" + +import time +from unittest.mock import MagicMock, Mock + +import pytest + +from nvflare.apis.analytix import AnalyticsDataType +from nvflare.apis.dxo import DXO, DataKind +from nvflare.apis.fl_constant import FLContextKey, ReservedKey, ReturnCode +from nvflare.apis.fl_context import FLContext +from nvflare.apis.shareable import Shareable +from nvflare.apis.signal import Signal +from nvflare.app_common.app_constant import AppConstants +from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext +from nvflare.app_common.executors.client_api.in_process_backend import InProcessBackend +from nvflare.app_common.executors.client_api_executor import ClientAPIExecutor +from nvflare.client.api_spec import CLIENT_API_KEY +from nvflare.client.config import ConfigKey, ExchangeFormat, TransferType +from nvflare.client.in_process.api import ( + TOPIC_ABORT, + TOPIC_GLOBAL_RESULT, + TOPIC_LOCAL_RESULT, + TOPIC_LOG_DATA, + TOPIC_STOP, + InProcessClientAPI, +) +from nvflare.fuel.data_event.data_bus import DataBus + +BACKEND_TOPICS = (TOPIC_LOCAL_RESULT, TOPIC_LOG_DATA, TOPIC_ABORT, TOPIC_STOP) + + +def _backend_callbacks_subscribed(bus, backend) -> bool: + """True if any of the BACKEND's own callbacks is still subscribed. + + Checked at callback granularity: the InProcessClientAPI the backend creates keeps its own + ABORT/STOP subscriptions (legacy behavior, unchanged for parity), so topic-level emptiness + would be the wrong assertion. + """ + backend_cbs = {backend._local_result_callback, backend._log_result_callback, backend._to_abort_callback} + for topic in BACKEND_TOPICS: + for cb, _ in bus.subscribers.get(topic, []): + if cb in backend_cbs: + return True + return False + + +@pytest.fixture(autouse=True) +def clean_databus(): + """The DataBus is a process singleton: isolate every test from prior subscriptions/data.""" + bus = DataBus() + bus.subscribers.clear() + bus.data_store.clear() + yield bus + bus.subscribers.clear() + bus.data_store.clear() + + +@pytest.fixture +def custom_dir(tmp_path): + """A workspace custom dir holding a trivial trainer script that exits immediately.""" + (tmp_path / "train.py").write_text("x = 1\n") + return str(tmp_path) + + +def _make_engine(): + """An engine whose new_context() works as a context manager (for the LOG callback).""" + engine = MagicMock() + engine.new_context.return_value.__enter__.return_value = FLContext() + return engine + + +def _make_fl_ctx(engine, custom_dir): + fl_ctx = FLContext() + fl_ctx.put(key=ReservedKey.ENGINE, value=engine, private=True, sticky=False) + fl_ctx.put(key=ReservedKey.RUN_NUM, value="job-1", private=False, sticky=False) + fl_ctx.put(key=ReservedKey.IDENTITY_NAME, value="site-1", private=False, sticky=False) + workspace = Mock() + workspace.get_app_custom_dir.return_value = custom_dir + fl_ctx.put(key=FLContextKey.WORKSPACE_OBJECT, value=workspace, private=True, sticky=False) + fl_ctx.put(key=FLContextKey.CURRENT_JOB_ID, value="job-1", private=False, sticky=False) + return fl_ctx + + +def _make_context(executor=None, **overrides): + kwargs = dict( + executor=executor if executor is not None else MagicMock(), + execution_mode="in_process", + task_script_path="train.py", + task_script_args="", + # bounded by default so a broken round trip FAILS the test instead of hanging it + result_wait_timeout=10.0, + ) + kwargs.update(overrides) + return ClientAPIBackendContext(**kwargs) + + +def _initialized_backend(custom_dir, executor=None, **overrides): + backend = InProcessBackend() + engine = _make_engine() + fl_ctx = _make_fl_ctx(engine, custom_dir) + backend.initialize(_make_context(executor=executor, **overrides), fl_ctx) + return backend, fl_ctx + + +def _result_shareable() -> Shareable: + return DXO(data_kind=DataKind.WEIGHTS, data={"w": [1.0]}).to_shareable() + + +class TestFactory: + def test_executor_factory_returns_in_process_backend(self): + executor = ClientAPIExecutor(execution_mode="in_process", task_script_path="custom/train.py") + backend = executor._create_in_process_backend() + assert isinstance(backend, InProcessBackend) + + def test_initialize_rejects_missing_or_non_py_script(self, custom_dir): + backend = InProcessBackend() + fl_ctx = _make_fl_ctx(_make_engine(), custom_dir) + for bad in (None, "", "train.sh"): + with pytest.raises(ValueError, match="task_script_path"): + backend.initialize(_make_context(task_script_path=bad), fl_ctx) + + +class TestInitializeAndFinalize: + def test_initialize_wires_databus_and_starts_trainer(self, clean_databus, custom_dir): + backend, _ = _initialized_backend(custom_dir) + try: + # the trainer script's flare.init() finds the API instance here + assert isinstance(clean_databus.get_data(CLIENT_API_KEY), InProcessClientAPI) + for topic in BACKEND_TOPICS: + assert topic in clean_databus.subscribers, f"backend must subscribe {topic}" + # the trivial script exits on its own + backend._task_fn_thread.join(timeout=5.0) + assert not backend._task_fn_thread.is_alive() + finally: + backend.finalize(FLContext()) + + def test_initialize_unwinds_on_failure(self, clean_databus, custom_dir): + backend = InProcessBackend() + engine = _make_engine() + fl_ctx = _make_fl_ctx(engine, custom_dir) + # break setup after the DataBus subscriptions happened + workspace = fl_ctx.get_prop(FLContextKey.WORKSPACE_OBJECT) + workspace.get_app_custom_dir.side_effect = RuntimeError("boom") + + with pytest.raises(RuntimeError, match="boom"): + backend.initialize(_make_context(), fl_ctx) + + # contract: self-unwinding - no backend subscriptions and no API instance left behind + assert clean_databus.get_data(CLIENT_API_KEY) is None + assert not _backend_callbacks_subscribed(clean_databus, backend) + + def test_finalize_idempotent_and_cleans_databus(self, clean_databus, custom_dir): + backend, _ = _initialized_backend(custom_dir) + fl_ctx = FLContext() + + backend.finalize(fl_ctx) + backend.finalize(fl_ctx) # contract: idempotent, must not raise + + assert clean_databus.get_data(CLIENT_API_KEY) is None + assert not _backend_callbacks_subscribed(clean_databus, backend) + assert not backend._task_fn_thread.is_alive() + + +class TestExecute: + def test_execute_round_trip(self, clean_databus, custom_dir): + backend, fl_ctx = _initialized_backend(custom_dir) + try: + result_to_send = _result_shareable() + + received_tasks = [] + + def fake_trainer(topic, data, databus): + # replies as flare.send() does: publish the result on TOPIC_LOCAL_RESULT. + # No asserts in here: a raise inside a DataBus callback is swallowed by its + # thread pool and would turn a failure into a timeout. + received_tasks.append(data) + clean_databus.publish([TOPIC_LOCAL_RESULT], result_to_send) + + clean_databus.subscribe([TOPIC_GLOBAL_RESULT], fake_trainer) + + task = Shareable() + task.set_header(AppConstants.CURRENT_ROUND, 3) + result = backend.execute("train", task, fl_ctx, Signal()) + + assert isinstance(result, Shareable) + assert result.get_return_code() == ReturnCode.OK + # the round travels back on the result for workflow bookkeeping + assert result.get_header(AppConstants.CURRENT_ROUND) == 3 + assert len(received_tasks) == 1 and isinstance(received_tasks[0], Shareable) + finally: + backend.finalize(FLContext()) + + def test_execute_returns_task_aborted_on_triggered_signal(self, clean_databus, custom_dir): + backend, fl_ctx = _initialized_backend(custom_dir) + try: + abort_events = [] + clean_databus.subscribe([TOPIC_ABORT], lambda t, d, b: abort_events.append(d)) + + signal = Signal() + signal.trigger("stop") + result = backend.execute("train", Shareable(), fl_ctx, signal) + + assert result.get_return_code() == ReturnCode.TASK_ABORTED + # the trainer was told the task is aborted + assert abort_events + finally: + backend.finalize(FLContext()) + + def test_execute_bounded_by_result_wait_timeout(self, clean_databus, custom_dir): + # contract: never wait unbounded past the configured bound; no trainer reply -> error + backend, fl_ctx = _initialized_backend(custom_dir, result_wait_timeout=0.05) + try: + start = time.time() + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + elapsed = time.time() - start + + assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + assert elapsed < 5.0, "timeout path must not hang" + finally: + backend.finalize(FLContext()) + + def test_multi_round_sequential_execute(self, clean_databus, custom_dir): + """The same backend serves consecutive rounds (the legacy executor's normal life).""" + backend, fl_ctx = _initialized_backend(custom_dir) + try: + clean_databus.subscribe( + [TOPIC_GLOBAL_RESULT], lambda t, d, b: clean_databus.publish([TOPIC_LOCAL_RESULT], _result_shareable()) + ) + for round_num in (0, 1, 2): + task = Shareable() + task.set_header(AppConstants.CURRENT_ROUND, round_num) + result = backend.execute("train", task, fl_ctx, Signal()) + assert result.get_return_code() == ReturnCode.OK + assert result.get_header(AppConstants.CURRENT_ROUND) == round_num + finally: + backend.finalize(FLContext()) + + def test_execute_after_timeout_fails_fast_with_accurate_rc(self, clean_databus, custom_dir): + """One result-wait timeout kills the in-process trainer for good (its thread is never + relaunched); later tasks must fail FAST with EXECUTION_EXCEPTION -- an instant + TASK_ABORTED would be misleading (task never delivered, abort_signal never triggered).""" + backend, fl_ctx = _initialized_backend(custom_dir, result_wait_timeout=0.05) + try: + first = backend.execute("train", Shareable(), fl_ctx, Signal()) + assert first.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + + start = time.monotonic() + second = backend.execute("train", Shareable(), fl_ctx, Signal()) + elapsed = time.monotonic() - start + assert second.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + assert elapsed < 1.0, "post-abort tasks must fail at entry, not wait the poll loop" + finally: + backend.finalize(FLContext()) + + def test_trainer_abort_mid_task_returns_task_aborted(self, clean_databus, custom_dir): + """TOPIC_ABORT arriving mid-wait (what TaskScriptRunner fires when the script raises) + aborts the current task well before the result-wait bound -- legacy parity.""" + backend, fl_ctx = _initialized_backend(custom_dir, result_wait_timeout=30.0) + try: + # deterministic: the abort fires synchronously right after the task is delivered + clean_databus.subscribe( + [TOPIC_GLOBAL_RESULT], lambda t, d, b: clean_databus.publish([TOPIC_ABORT], "script failure") + ) + start = time.monotonic() + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + elapsed = time.monotonic() - start + + assert result.get_return_code() == ReturnCode.TASK_ABORTED + assert elapsed < 10.0, "abort must not wait out the result bound" + finally: + backend.finalize(FLContext()) + + def test_bad_result_type_returns_execution_exception(self, clean_databus, custom_dir): + backend, fl_ctx = _initialized_backend(custom_dir) + try: + clean_databus.subscribe( + [TOPIC_GLOBAL_RESULT], + lambda t, d, b: clean_databus.publish([TOPIC_LOCAL_RESULT], "not-a-shareable"), + ) + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + finally: + backend.finalize(FLContext()) + + +class TestMultiBackend: + def test_finalizing_backend_does_not_clear_successors_client_api(self, clean_databus, custom_dir): + """Backend A's teardown must not remove the CLIENT_API_KEY a later backend B installed.""" + backend_a, _ = _initialized_backend(custom_dir) + backend_b, _ = _initialized_backend(custom_dir) + try: + api_b = backend_b._client_api + assert clean_databus.get_data(CLIENT_API_KEY) is api_b + + backend_a.finalize(FLContext()) + assert clean_databus.get_data(CLIENT_API_KEY) is api_b + finally: + backend_b.finalize(FLContext()) + assert clean_databus.get_data(CLIENT_API_KEY) is None + + def test_finalize_fires_stop_exactly_once(self, clean_databus, custom_dir): + """Idempotency means no repeated side effects, not just no raise.""" + backend, _ = _initialized_backend(custom_dir) + stops = [] + clean_databus.subscribe([TOPIC_STOP], lambda t, d, b: stops.append(d)) + + backend.finalize(FLContext()) + backend.finalize(FLContext()) + + assert len(stops) == 1 + + def test_finalize_detaches_api_own_subscriptions(self, clean_databus, custom_dir): + """The API instance's own bus subscriptions go away with the backend (close()), so a + dead job's API can no longer receive -- and pin -- a later job's global model.""" + backend, _ = _initialized_backend(custom_dir) + api = backend._client_api + + backend.finalize(FLContext()) + + for topic, subs in clean_databus.subscribers.items(): + for cb, _kw in subs: + assert getattr(cb, "__self__", None) is not api, f"dead API still subscribed on {topic}" + + def test_initialize_unwinds_on_late_failure(self, clean_databus, custom_dir, monkeypatch): + """Failure AFTER the API instance exists: its subscriptions must not leak either.""" + + def raising_init(self, *args, **kwargs): + raise RuntimeError("late boom") + + monkeypatch.setattr(InProcessClientAPI, "init", raising_init) + backend = InProcessBackend() + fl_ctx = _make_fl_ctx(_make_engine(), custom_dir) + + with pytest.raises(RuntimeError, match="late boom"): + backend.initialize(_make_context(), fl_ctx) + + assert clean_databus.get_data(CLIENT_API_KEY) is None + assert not _backend_callbacks_subscribed(clean_databus, backend) + for topic, subs in clean_databus.subscribers.items(): + for cb, _kw in subs: + assert not isinstance(getattr(cb, "__self__", None), InProcessClientAPI), f"leak on {topic}" + + +class TestLogRouting: + def test_log_data_routes_through_executor_fire_log_analytics(self, clean_databus, custom_dir): + executor = MagicMock() + backend, _ = _initialized_backend(custom_dir, executor=executor) + try: + # what flare.log() publishes: a dict with key/value/data_type + clean_databus.publish( + [TOPIC_LOG_DATA], {"key": "accuracy", "value": 0.9, "data_type": AnalyticsDataType.SCALAR} + ) + + assert executor.fire_log_analytics.call_count == 1 + dxo = executor.fire_log_analytics.call_args[0][1] + # the key->tag rename happened and the DXO carries the exact metric + assert dxo.data == {"track_key": "accuracy", "track_value": 0.9} + finally: + backend.finalize(FLContext()) + + +class TestMetaPassThrough: + def test_meta_uses_raw_full_and_context_task_names(self, custom_dir): + backend, fl_ctx = _initialized_backend( + custom_dir, train_task_name="my_train", evaluate_task_name="my_eval", submit_model_task_name="my_submit" + ) + try: + meta = backend._prepare_task_meta(fl_ctx, "my_train") + exchange = meta[ConfigKey.TASK_EXCHANGE] + # FLARE-2698: pass-through boundary - no converter formats in the frozen surface + assert exchange[ConfigKey.EXCHANGE_FORMAT] == ExchangeFormat.RAW + assert exchange[ConfigKey.TRANSFER_TYPE] == TransferType.FULL + assert exchange[ConfigKey.TRAIN_TASK_NAME] == "my_train" + assert exchange[ConfigKey.EVAL_TASK_NAME] == "my_eval" + assert exchange[ConfigKey.SUBMIT_MODEL_TASK_NAME] == "my_submit" + assert meta[ConfigKey.TASK_NAME] == "my_train" + finally: + backend.finalize(FLContext()) From 94627411f14a792398650fb46e5ac7ba320097a2 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Tue, 7 Jul 2026 18:44:24 -0700 Subject: [PATCH 04/12] Address ClientAPIExecutor review findings --- .../executors/client_api/backend_spec.py | 28 +++--- .../client_api/in_process_backend.py | 50 ++++++++--- .../executors/client_api_executor.py | 9 +- .../executors/client_api_executor_test.py | 46 +++++----- .../executors/in_process_backend_test.py | 85 +++++++++++++++++-- 5 files changed, 156 insertions(+), 62 deletions(-) diff --git a/nvflare/app_common/executors/client_api/backend_spec.py b/nvflare/app_common/executors/client_api/backend_spec.py index 5893ceea8d..9378d05c3d 100644 --- a/nvflare/app_common/executors/client_api/backend_spec.py +++ b/nvflare/app_common/executors/client_api/backend_spec.py @@ -43,11 +43,11 @@ class ClientAPIBackendContext: """Immutable config a ClientAPIExecutor hands to its backend at ``initialize()``. - Rationale (finding 11): the executor's frozen constructor args live in private attributes and - the backend factories are zero-arg, so a backend previously had no clean way to read the - heartbeat/timeout/converter/task-name config it needs, nor a supported reference back to the - executor's analytics hook. This frozen snapshot is that supported channel - a backend reads its - config from here rather than reaching into ``ClientAPIExecutor`` private attributes. + Rationale: the executor's frozen constructor args live in private attributes and the backend + factories are zero-arg, so a backend previously had no clean way to read the heartbeat/timeout/ + converter/task-name config it needs, nor a supported reference back to the executor's analytics + hook. This frozen snapshot is that supported channel - a backend reads its config from here + rather than reaching into ``ClientAPIExecutor`` private attributes. The fields mirror the frozen ``ClientAPIExecutor`` constructor surface one-to-one. ``executor`` is a back-reference so a backend can: @@ -111,10 +111,10 @@ class ClientAPIBackendSpec(ABC): def initialize(self, context: ClientAPIBackendContext, fl_ctx: FLContext) -> None: """Prepares the backend for the run. Called once when the executor handles START_RUN. - ``context`` (finding 11) is the frozen snapshot of the executor's configuration plus a - back-reference to the executor (for ``fire_log_analytics`` and logging). A backend should - read all of its config from ``context`` rather than from executor private attributes, and - should retain what it needs for ``execute``/``handle_event``/``finalize``. + ``context`` is the frozen snapshot of the executor's configuration plus a back-reference to + the executor (for ``fire_log_analytics`` and logging). A backend should read all of its + config from ``context`` rather than from executor private attributes, and should retain what + it needs for ``execute``/``handle_event``/``finalize``. The backend sets up its control plane here (DataBus wiring for in_process; Cell session machinery, bootstrap config, and - per launch_once policy - trainer launch for @@ -124,11 +124,11 @@ def initialize(self, context: ClientAPIBackendContext, fl_ctx: FLContext) -> Non into system_panic so the job fails cleanly instead of hanging while tasks wait on a backend that never became ready. - Cleanup-on-failure contract (finding 12): ``initialize()`` must be exception-safe and - self-unwinding - if it raises, it must first release any partial setup it already made - (threads started, processes launched, listeners/tokens registered, files written). The - executor does NOT call ``finalize()`` on a backend whose ``initialize()`` raised, because - ``finalize()`` cannot assume a consistently half-initialized backend. Own your own rollback. + Cleanup-on-failure contract: ``initialize()`` must be exception-safe and self-unwinding - if + it raises, it must first release any partial setup it already made (threads started, + processes launched, listeners/tokens registered, files written). The executor does NOT call + ``finalize()`` on a backend whose ``initialize()`` raised, because ``finalize()`` cannot + assume a consistently half-initialized backend. Own your own rollback. Args: context: the frozen backend configuration and executor back-reference. diff --git a/nvflare/app_common/executors/client_api/in_process_backend.py b/nvflare/app_common/executors/client_api/in_process_backend.py index ac016cecc7..4d4c3c03ec 100644 --- a/nvflare/app_common/executors/client_api/in_process_backend.py +++ b/nvflare/app_common/executors/client_api/in_process_backend.py @@ -43,6 +43,7 @@ from nvflare.apis.fl_constant import FLContextKey, FLMetaKey, ReturnCode from nvflare.apis.fl_context import FLContext +from nvflare.apis.fl_exception import UnsafeJobError from nvflare.apis.shareable import Shareable, make_reply from nvflare.apis.signal import Signal from nvflare.apis.utils.analytix_utils import create_analytic_dxo @@ -69,6 +70,7 @@ # The legacy executor exposed this as result_pull_interval (default 0.5); the frozen # surface deliberately drops the knob, so the default becomes the behavior. _RESULT_POLL_INTERVAL = 0.5 +_NO_RESULT = object() class InProcessBackend(ClientAPIBackendSpec): @@ -85,7 +87,7 @@ def __init__(self): self._event_manager: Optional[EventManager] = None self._client_api: Optional[InProcessClientAPI] = None self._task_fn_thread: Optional[threading.Thread] = None - self._local_result: Optional[Shareable] = None + self._local_result = _NO_RESULT self._abort = False self._abort_reason: Optional[str] = None self._finalized = False @@ -142,6 +144,9 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort self._event_manager.fire_event(TOPIC_ABORT, f"'{task_name}' is aborted, abort_signal_triggered") return make_reply(ReturnCode.TASK_ABORTED) + if not self._trainer_thread_is_alive(): + self._latch_abort("trainer thread exited without signaling ABORT") + if self._abort: # An in-process trainer that aborted (script failure, prior timeout, STOP) is gone # for good -- the thread is never relaunched. Fail fast with an accurate return code: @@ -159,7 +164,7 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort # concurrently with that task's timeout decision must not satisfy this task's wait. # No further correlation is needed: execute() runs one task at a time, and any # trainer abort latches _abort above, so at most one such straggler can exist. - self._local_result = None + self._local_result = _NO_RESULT try: # kept from the legacy executor: some task scripts read this ad-hoc prop @@ -178,6 +183,7 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort # monotonic: a wall-clock step (NTP, VM resume) must not fire a spurious timeout, # which would kill the trainer for the rest of the job wait_start = time.monotonic() + wait_deadline = None if result_wait_timeout is None else wait_start + result_wait_timeout self._context.executor.log_info(fl_ctx, "waiting for result from in-process trainer") while True: if abort_signal.triggered or self._abort: @@ -185,9 +191,9 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort self._event_manager.fire_event(TOPIC_ABORT, f"'{task_name}' is aborted, abort_signal_triggered") return make_reply(ReturnCode.TASK_ABORTED) - if self._local_result is not None: + if self._local_result is not _NO_RESULT: result = self._local_result - self._local_result = None + self._local_result = _NO_RESULT if not isinstance(result, Shareable): self._context.executor.log_error( @@ -200,7 +206,14 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort result.set_header(AppConstants.CURRENT_ROUND, current_round) return result - if result_wait_timeout is not None and time.monotonic() - wait_start > result_wait_timeout: + if not self._trainer_thread_is_alive(): + reason = "trainer thread exited before producing a result" + self._latch_abort(reason) + self._context.executor.log_error(fl_ctx, f"{reason} for task '{task_name}'") + return make_reply(ReturnCode.EXECUTION_EXCEPTION) + + now = time.monotonic() + if wait_deadline is not None and now >= wait_deadline: # the contract forbids waiting unbounded past the configured bound: tell the # trainer to stop this task and fail the round self._event_manager.fire_event( @@ -211,7 +224,12 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort ) return make_reply(ReturnCode.EXECUTION_EXCEPTION) - time.sleep(_RESULT_POLL_INTERVAL) + sleep_time = _RESULT_POLL_INTERVAL + if wait_deadline is not None: + sleep_time = min(sleep_time, wait_deadline - now) + time.sleep(sleep_time) + except UnsafeJobError: + raise except Exception: self._context.executor.log_error(fl_ctx, secure_format_traceback()) self._event_manager.fire_event(TOPIC_ABORT, f"'{task_name}' failed: {secure_format_traceback()}") @@ -283,6 +301,15 @@ def _prepare_task_meta(self, fl_ctx: FLContext, task_name: Optional[str]) -> dic }, } + def _trainer_thread_is_alive(self) -> bool: + thread = self._task_fn_thread + return thread is not None and thread.is_alive() + + def _latch_abort(self, reason: str) -> None: + self._abort = True + if self._abort_reason is None: + self._abort_reason = reason + def _local_result_callback(self, topic, data, databus): if not isinstance(data, Shareable): # do not raise into the trainer's send path; record the bad result and let the @@ -293,8 +320,9 @@ def _local_result_callback(self, topic, data, databus): def _log_result_callback(self, topic, data, databus): result = data - if result and not isinstance(result, dict): - raise ValueError(f"invalid result format, expecting Dict, but get {type(result)}") + if not isinstance(result, dict): + self.logger.error(f"invalid result format, expecting Dict, but got {type(result)}") + return if "key" in result: result["tag"] = result.pop("key") @@ -305,7 +333,5 @@ def _log_result_callback(self, topic, data, databus): self._context.executor.fire_log_analytics(fl_ctx, dxo) def _to_abort_callback(self, topic, data, databus): - self._abort = True - if self._abort_reason is None: - # keep the FIRST cause: later echoes (e.g. our own fail-fast fires) must not mask it - self._abort_reason = f"{topic}: {data}" + # keep the FIRST cause: later echoes (e.g. our own fail-fast fires) must not mask it + self._latch_abort(f"{topic}: {data}") diff --git a/nvflare/app_common/executors/client_api_executor.py b/nvflare/app_common/executors/client_api_executor.py index df31b4ea30..a2f988e30b 100644 --- a/nvflare/app_common/executors/client_api_executor.py +++ b/nvflare/app_common/executors/client_api_executor.py @@ -42,7 +42,6 @@ edge (the intermediate layers pass through), so these are not frozen into this surface. The transfer type (FULL/DIFF) remains a Client API concern (model_registry) and is decided separately from the converter removal. - (public on ScriptRunner and both legacy executors). """ from typing import Callable, Dict, Optional @@ -278,9 +277,9 @@ def handle_event(self, event_type: str, fl_ctx: FLContext): self._backend = self._create_backend() self._backend.initialize(self._build_backend_context(), fl_ctx) except Exception as e: - # finding 12: initialize() is contracted to self-unwind its partial setup on - # failure, so the executor does NOT call finalize() on a half-initialized backend; - # it just drops the reference and panics so the job fails cleanly. + # initialize() is contracted to self-unwind its partial setup on failure, so the + # executor does NOT call finalize() on a half-initialized backend; it just drops + # the reference and panics so the job fails cleanly. self._backend = None self.log_error(fl_ctx, secure_format_traceback(), fire_event=False) self.system_panic( @@ -395,7 +394,7 @@ def _wrong_mode_error(arg_name: str, value, valid_modes: str, execution_mode: st ) def _build_backend_context(self) -> ClientAPIBackendContext: - """Builds the frozen config snapshot handed to the backend at initialize() (finding 11).""" + """Builds the frozen config snapshot handed to the backend at initialize().""" return ClientAPIBackendContext( executor=self, execution_mode=self._execution_mode, diff --git a/tests/unit_test/app_common/executors/client_api_executor_test.py b/tests/unit_test/app_common/executors/client_api_executor_test.py index cd113c9283..93084e0ba0 100644 --- a/tests/unit_test/app_common/executors/client_api_executor_test.py +++ b/tests/unit_test/app_common/executors/client_api_executor_test.py @@ -170,8 +170,8 @@ def test_valid_full_surface(self): assert executor._cuda_empty_cache is True def test_task_name_and_memory_defaults(self): - # findings 9 + 10: rank-contract task names and memory-management knobs are frozen with the - # legacy executors' defaults, valid in every mode. + # Rank-contract task names and memory-management knobs are frozen with the legacy + # executors' defaults, valid in every mode. executor = ClientAPIExecutor(execution_mode="in_process") assert executor._train_task_name == AppConstants.TASK_TRAIN assert executor._evaluate_task_name == AppConstants.TASK_VALIDATION @@ -181,8 +181,8 @@ def test_task_name_and_memory_defaults(self): assert executor._cuda_empty_cache is False def test_in_process_accepts_task_script(self): - # finding 8: in_process names its script via task_script_path/args (command names the - # external_process trainer). + # in_process names its script via task_script_path/args; command names the external_process + # trainer. executor = ClientAPIExecutor( execution_mode="in_process", task_script_path="custom/train.py", task_script_args="--epochs 3" ) @@ -238,20 +238,20 @@ def test_allow_reconnect_rejected_for_non_attach_modes(self, kwargs): @pytest.mark.parametrize("mode", ["in_process", "attach"]) @pytest.mark.parametrize("command", ["", " ", "\t"]) def test_empty_command_treated_as_unset_for_non_external_modes(self, mode, command): - # finding 1: an empty/whitespace command is "unset", not a wrong-mode command, so it must - # not be rejected with the misleading "only valid for external_process" message. + # An empty/whitespace command is "unset", not a wrong-mode command, so it must not be + # rejected with the misleading "only valid for external_process" message. executor = ClientAPIExecutor(execution_mode=mode, command=command) assert executor._command is None @pytest.mark.parametrize("command", ["", " "]) def test_external_process_rejects_empty_command(self, command): - # finding 1: the same normalization must keep external_process requiring a real command. + # The same normalization must keep external_process requiring a real command. with pytest.raises(ValueError, match="'external_process' requires a non-empty command"): ClientAPIExecutor(execution_mode="external_process", command=command) @pytest.mark.parametrize("mode", ["external_process", "attach"]) def test_task_script_path_rejected_for_non_in_process_modes(self, mode): - # finding 8: task_script_path names the in_process script only. + # task_script_path names the in_process script only. kwargs = dict(MODE_KWARGS[mode]) with pytest.raises(ValueError, match="task_script_path is only valid for execution_mode 'in_process'"): ClientAPIExecutor(task_script_path="custom/train.py", **kwargs) @@ -264,8 +264,8 @@ def test_task_script_args_rejected_for_non_in_process_modes(self, mode): @pytest.mark.parametrize("mode", ["in_process", "attach"]) def test_launch_once_non_default_rejected_for_non_external_modes(self, mode): - # finding 3: external_process-only knobs must be rejected (not silently ignored) when set - # to a non-default value in a mode that ignores them. + # external_process-only knobs must be rejected (not silently ignored) when set to a + # non-default value in a mode that ignores them. kwargs = dict(MODE_KWARGS[mode]) with pytest.raises(ValueError, match="launch_once is only valid for execution_mode 'external_process'"): ClientAPIExecutor(launch_once=False, **kwargs) @@ -290,8 +290,8 @@ def test_stop_grace_period_non_default_rejected_for_non_external_modes(self, mod @pytest.mark.parametrize("arg", ["heartbeat_interval", "heartbeat_timeout"]) def test_heartbeat_non_default_rejected_for_in_process(self, arg): - # finding 3: there is no session heartbeat in_process, so a non-default heartbeat knob is - # dead there and must be rejected (valid for external_process/attach). + # There is no session heartbeat in_process, so a non-default heartbeat knob is dead there + # and must be rejected (valid for external_process/attach). with pytest.raises(ValueError, match=f"{arg} is only valid for execution_mode 'external_process' or 'attach'"): ClientAPIExecutor(execution_mode="in_process", **{arg: 99.0}) @@ -319,9 +319,9 @@ def test_allow_reconnect_default_value_accepted_for_non_attach_modes(self, mode) @pytest.mark.parametrize("mode", ["in_process", "external_process"]) @pytest.mark.parametrize("falsy", [None, 0]) def test_allow_reconnect_falsy_values_accepted_for_non_attach_modes(self, mode, falsy): - # finding 2: the wrong-mode check uses `if allow_reconnect` (truthy), not - # `is not False`, so falsy-but-not-False values (None, 0, numpy.bool_(False)) are treated - # as "not set" instead of misfiring. + # The wrong-mode check uses `if allow_reconnect` (truthy), not `is not False`, so + # falsy-but-not-False values (None, 0, numpy.bool_(False)) are treated as "not set" + # instead of misfiring. kwargs = dict(MODE_KWARGS[mode]) ClientAPIExecutor(allow_reconnect=falsy, **kwargs) @@ -343,8 +343,8 @@ def test_in_process_factory_returns_real_backend(self): @pytest.mark.parametrize("mode", NOT_IMPLEMENTED_MODES) def test_backend_factory_raises_not_implemented(self, mode): - # finding 7: user-facing message must not carry an internal plan id (EX-3/EP-4/AT-2); it - # names the mode and says "not yet implemented". + # The user-facing message must not carry an internal plan id (EX-3/EP-4/AT-2); it names the + # mode and says "not yet implemented". executor = ClientAPIExecutor(**MODE_KWARGS[mode]) with pytest.raises(NotImplementedError, match="not yet implemented") as exc_info: executor._create_backend() @@ -355,8 +355,8 @@ def test_backend_factory_raises_not_implemented(self, mode): @pytest.mark.parametrize("mode", list(ALL_EXECUTION_MODES)) def test_start_run_panics_naming_the_mode(self, mode): - # findings 6 + 7: the panic reason names the mode directly (not via secure_format_exception, - # so it is robust whether or not NVFLARE_SECURE_LOGGING is set) and carries no plan id. + # The panic reason names the mode directly (not via secure_format_exception, so it is robust + # whether or not NVFLARE_SECURE_LOGGING is set) and carries no plan id. # For in_process the failure is now the missing task_script_path (backend initialize # raises), not a NotImplementedError factory - the clean-failure contract is the same. executor = ClientAPIExecutor(**MODE_KWARGS[mode]) @@ -457,8 +457,8 @@ def test_backend_non_shareable_result_becomes_error_reply(self): assert reply.get_return_code() == ReturnCode.EXECUTION_EXCEPTION def test_backend_receives_config_context(self): - # finding 11: initialize() receives a frozen ClientAPIBackendContext carrying the executor - # config and a back-reference to the executor (for fire_log_analytics/logging). + # initialize() receives a frozen ClientAPIBackendContext carrying the executor config and a + # back-reference to the executor (for fire_log_analytics/logging). backend = _StubBackend() executor = _StubbedInProcessExecutor( backend, @@ -513,8 +513,8 @@ def test_default_path_fires_local_analytics_event(self): assert scopes == [(ANALYTIC_EVENT_TYPE, EventScope.LOCAL)] def test_fed_path_fires_federation_scoped_event(self): - # finding 4: the fed path must fire the already-"fed."-prefixed event name so it lands on - # the same server-side event as MetricRelay (job_config/script_runner.py) and flower_job.py + # The fed path must fire the already-"fed."-prefixed event name so it lands on the same + # server-side event as MetricRelay (job_config/script_runner.py) and flower_job.py # ("fed.analytix_log_stats"); firing the un-prefixed name federation-scoped would miss every # consumer listening on "fed.analytix_log_stats". assert FED_ANALYTIC_EVENT_TYPE == "fed.analytix_log_stats" diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py index 42063b53d5..26b1c4a1d3 100644 --- a/tests/unit_test/app_common/executors/in_process_backend_test.py +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -31,6 +31,7 @@ from nvflare.apis.dxo import DXO, DataKind from nvflare.apis.fl_constant import FLContextKey, ReservedKey, ReturnCode from nvflare.apis.fl_context import FLContext +from nvflare.apis.fl_exception import UnsafeJobError from nvflare.apis.shareable import Shareable from nvflare.apis.signal import Signal from nvflare.app_common.app_constant import AppConstants @@ -80,7 +81,23 @@ def clean_databus(): @pytest.fixture def custom_dir(tmp_path): - """A workspace custom dir holding a trivial trainer script that exits immediately.""" + """A workspace custom dir holding a trainer thread that stays alive until STOP.""" + (tmp_path / "train.py").write_text( + "from nvflare.client.api_spec import CLIENT_API_KEY\n" + "from nvflare.fuel.data_event.data_bus import DataBus\n" + "api = DataBus().get_data(CLIENT_API_KEY)\n" + "try:\n" + " while api.is_running():\n" + " api.clear()\n" + "except RuntimeError:\n" + " pass\n" + ) + return str(tmp_path) + + +@pytest.fixture +def exited_custom_dir(tmp_path): + """A workspace custom dir holding a trainer script that exits normally.""" (tmp_path / "train.py").write_text("x = 1\n") return str(tmp_path) @@ -151,11 +168,10 @@ def test_initialize_wires_databus_and_starts_trainer(self, clean_databus, custom assert isinstance(clean_databus.get_data(CLIENT_API_KEY), InProcessClientAPI) for topic in BACKEND_TOPICS: assert topic in clean_databus.subscribers, f"backend must subscribe {topic}" - # the trivial script exits on its own - backend._task_fn_thread.join(timeout=5.0) - assert not backend._task_fn_thread.is_alive() + assert backend._task_fn_thread.is_alive() finally: backend.finalize(FLContext()) + assert not backend._task_fn_thread.is_alive() def test_initialize_unwinds_on_failure(self, clean_databus, custom_dir): backend = InProcessBackend() @@ -229,16 +245,33 @@ def test_execute_returns_task_aborted_on_triggered_signal(self, clean_databus, c finally: backend.finalize(FLContext()) - def test_execute_bounded_by_result_wait_timeout(self, clean_databus, custom_dir): + def test_execute_bounded_by_result_wait_timeout(self, clean_databus, custom_dir, monkeypatch): # contract: never wait unbounded past the configured bound; no trainer reply -> error + monkeypatch.setattr("nvflare.app_common.executors.client_api.in_process_backend._RESULT_POLL_INTERVAL", 1.0) backend, fl_ctx = _initialized_backend(custom_dir, result_wait_timeout=0.05) try: - start = time.time() + start = time.monotonic() + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + elapsed = time.monotonic() - start + + assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + assert elapsed < 0.5, "timeout must not be rounded up to the polling interval" + finally: + backend.finalize(FLContext()) + + def test_execute_fails_fast_when_trainer_thread_exits(self, clean_databus, exited_custom_dir): + backend, fl_ctx = _initialized_backend(exited_custom_dir, result_wait_timeout=2.0) + try: + backend._task_fn_thread.join(timeout=1.0) + assert not backend._task_fn_thread.is_alive() + + start = time.monotonic() result = backend.execute("train", Shareable(), fl_ctx, Signal()) - elapsed = time.time() - start + elapsed = time.monotonic() - start assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION - assert elapsed < 5.0, "timeout path must not hang" + assert elapsed < 0.5, "a dead trainer must be detected before waiting for a result" + assert "trainer thread exited" in backend._abort_reason finally: backend.finalize(FLContext()) @@ -305,6 +338,31 @@ def test_bad_result_type_returns_execution_exception(self, clean_databus, custom finally: backend.finalize(FLContext()) + def test_none_result_returns_execution_exception_without_waiting_for_timeout(self, clean_databus, custom_dir): + backend, fl_ctx = _initialized_backend(custom_dir, result_wait_timeout=1.0) + try: + clean_databus.subscribe( + [TOPIC_GLOBAL_RESULT], + lambda t, d, b: clean_databus.publish([TOPIC_LOCAL_RESULT], None), + ) + start = time.monotonic() + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + elapsed = time.monotonic() - start + + assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + assert elapsed < 0.5, "an invalid None result must fail fast" + finally: + backend.finalize(FLContext()) + + def test_unsafe_job_error_propagates(self, clean_databus, custom_dir, monkeypatch): + backend, fl_ctx = _initialized_backend(custom_dir) + try: + monkeypatch.setattr(backend, "_prepare_task_meta", Mock(side_effect=UnsafeJobError("unsafe"))) + with pytest.raises(UnsafeJobError, match="unsafe"): + backend.execute("train", Shareable(), fl_ctx, Signal()) + finally: + backend.finalize(FLContext()) + class TestMultiBackend: def test_finalizing_backend_does_not_clear_successors_client_api(self, clean_databus, custom_dir): @@ -381,6 +439,17 @@ def test_log_data_routes_through_executor_fire_log_analytics(self, clean_databus finally: backend.finalize(FLContext()) + def test_invalid_log_data_is_logged_and_ignored(self, clean_databus, custom_dir, caplog): + executor = MagicMock() + backend, _ = _initialized_backend(custom_dir, executor=executor) + try: + backend._log_result_callback(TOPIC_LOG_DATA, None, clean_databus) + + executor.fire_log_analytics.assert_not_called() + assert "invalid result format" in caplog.text + finally: + backend.finalize(FLContext()) + class TestMetaPassThrough: def test_meta_uses_raw_full_and_context_task_names(self, custom_dir): From be556448a7fed946258e3f150df34650b013ade9 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Tue, 7 Jul 2026 19:18:47 -0700 Subject: [PATCH 05/12] Address remaining ClientAPIExecutor review comments --- .../executors/client_api/backend_spec.py | 13 ++++++------ .../client_api/in_process_backend.py | 17 ++++++++++------ .../executors/client_api_executor.py | 20 +++++++++++++++---- .../executors/client_api_executor_test.py | 5 +++-- .../executors/in_process_backend_test.py | 15 ++++++++++++++ 5 files changed, 52 insertions(+), 18 deletions(-) diff --git a/nvflare/app_common/executors/client_api/backend_spec.py b/nvflare/app_common/executors/client_api/backend_spec.py index 9378d05c3d..8874b4e1f2 100644 --- a/nvflare/app_common/executors/client_api/backend_spec.py +++ b/nvflare/app_common/executors/client_api/backend_spec.py @@ -32,6 +32,7 @@ from nvflare.apis.fl_context import FLContext from nvflare.apis.shareable import Shareable from nvflare.apis.signal import Signal +from nvflare.app_common.app_constant import AppConstants if TYPE_CHECKING: # Import for typing only; avoids a runtime import cycle @@ -54,9 +55,9 @@ class ClientAPIBackendContext: - call ``executor.fire_log_analytics(fl_ctx, dxo)`` for every trainer LOG message (the single LOG-to-analytics ownership point; see design "Configuration Surface"), and - - select the federation-scoped analytics path when appropriate by setting - ``executor._analytics_fire_fed_event = True`` in ``initialize()`` (Cell backends do this when - no ConvertToFedEvent widget is configured), and + - select the federation-scoped analytics path when appropriate by calling + ``executor.set_analytics_fire_fed_event(True)`` in ``initialize()`` (Cell backends do this + when no ConvertToFedEvent widget is configured), and - use the executor's FLComponent logging helpers. """ @@ -83,9 +84,9 @@ class ClientAPIBackendContext: # filters at the client edge; the Client API boundary is pass-through. Transfer type # (FULL/DIFF) stays a Client API concern (model_registry), decided separately. # task-name / rank contract (all modes) - train_task_name: str = "train" - evaluate_task_name: str = "validate" - submit_model_task_name: str = "submit_model" + train_task_name: str = AppConstants.TASK_TRAIN + evaluate_task_name: str = AppConstants.TASK_VALIDATION + submit_model_task_name: str = AppConstants.TASK_SUBMIT_MODEL train_with_evaluation: bool = False # memory management (all modes) memory_gc_rounds: int = 0 diff --git a/nvflare/app_common/executors/client_api/in_process_backend.py b/nvflare/app_common/executors/client_api/in_process_backend.py index 4d4c3c03ec..935159528e 100644 --- a/nvflare/app_common/executors/client_api/in_process_backend.py +++ b/nvflare/app_common/executors/client_api/in_process_backend.py @@ -324,13 +324,18 @@ def _log_result_callback(self, topic, data, databus): self.logger.error(f"invalid result format, expecting Dict, but got {type(result)}") return - if "key" in result: - result["tag"] = result.pop("key") - dxo = create_analytic_dxo(**result) + try: + if "key" in result: + result["tag"] = result.pop("key") + dxo = create_analytic_dxo(**result) - # single analytics-event ownership point: the executor decides local vs fed fire path - with self._engine.new_context() as fl_ctx: - self._context.executor.fire_log_analytics(fl_ctx, dxo) + # single analytics-event ownership point: the executor decides local vs fed fire path + with self._engine.new_context() as fl_ctx: + self._context.executor.fire_log_analytics(fl_ctx, dxo) + except Exception: + # DataBus callback failures otherwise disappear in the thread-pool Future, dropping the + # metric without any useful diagnostic. + self.logger.error(f"failed to process trainer LOG data: {secure_format_traceback()}") def _to_abort_callback(self, topic, data, databus): # keep the FIRST cause: later echoes (e.g. our own fail-fast fires) must not mask it diff --git a/nvflare/app_common/executors/client_api_executor.py b/nvflare/app_common/executors/client_api_executor.py index a2f988e30b..ae0d5f37e7 100644 --- a/nvflare/app_common/executors/client_api_executor.py +++ b/nvflare/app_common/executors/client_api_executor.py @@ -257,7 +257,7 @@ def __init__( self._memory_gc_rounds = memory_gc_rounds self._cuda_empty_cache = cuda_empty_cache self._attach_timeout = attach_timeout - self._allow_reconnect = allow_reconnect + self._allow_reconnect = bool(allow_reconnect) self._backend: Optional[ClientAPIBackendSpec] = None @@ -335,6 +335,17 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort return make_reply(ReturnCode.EXECUTION_EXCEPTION) return result + def set_analytics_fire_fed_event(self, enabled: bool) -> None: + """Selects whether trainer LOG data is fired as a federation-scoped analytics event. + + Cell backends call this during initialization when no ConvertToFedEvent widget is + configured. The default remains the local analytics path used by the in-process backend. + + Args: + enabled: True to fire federation-scoped events directly; False to fire local events. + """ + self._analytics_fire_fed_event = bool(enabled) + def fire_log_analytics(self, fl_ctx: FLContext, dxo: DXO) -> None: """Converts trainer LOG data into an analytics event. Executor-owned surface. @@ -342,14 +353,15 @@ def fire_log_analytics(self, fl_ctx: FLContext, dxo: DXO) -> None: regardless of execution mode - this replaces MetricRelay (ex-process) and the in-process executor's log callback as the single analytics-event ownership point. - The fire path is mode-selectable via ``self._analytics_fire_fed_event``: + The fire path is mode-selectable via ``set_analytics_fire_fed_event()``: - False (default): fires the local, un-prefixed ANALYTIC_EVENT_TYPE ("analytix_log_stats") and relies on the ConvertToFedEvent widget (added by BaseFedJob) to forward it to the server as "fed.analytix_log_stats". - True: fires a federation-scoped event directly to the server, matching what - MetricRelay does today for ex-process metrics. Cell backends may select this - path during initialize() when no ConvertToFedEvent widget is configured. + MetricRelay does today for ex-process metrics. Cell backends may select this path during + initialize() by calling ``set_analytics_fire_fed_event(True)`` when no ConvertToFedEvent + widget is configured. The two paths must land on the same server-side event name. ConvertToFedEvent prefixes the local event with "fed.", so the fed path must fire the already-prefixed diff --git a/tests/unit_test/app_common/executors/client_api_executor_test.py b/tests/unit_test/app_common/executors/client_api_executor_test.py index 93084e0ba0..792a5f382a 100644 --- a/tests/unit_test/app_common/executors/client_api_executor_test.py +++ b/tests/unit_test/app_common/executors/client_api_executor_test.py @@ -323,7 +323,8 @@ def test_allow_reconnect_falsy_values_accepted_for_non_attach_modes(self, mode, # falsy-but-not-False values (None, 0, numpy.bool_(False)) are treated as "not set" # instead of misfiring. kwargs = dict(MODE_KWARGS[mode]) - ClientAPIExecutor(allow_reconnect=falsy, **kwargs) + executor = ClientAPIExecutor(allow_reconnect=falsy, **kwargs) + assert executor._allow_reconnect is False class TestDispatch: @@ -495,7 +496,7 @@ class TestAnalyticsOwnership: def _fire(self, fire_fed_event: bool): executor = ClientAPIExecutor(execution_mode="in_process") - executor._analytics_fire_fed_event = fire_fed_event + executor.set_analytics_fire_fed_event(fire_fed_event) engine = Mock() scopes = [] engine.fire_event.side_effect = lambda event_type, ctx: scopes.append( diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py index 26b1c4a1d3..f17ba5b690 100644 --- a/tests/unit_test/app_common/executors/in_process_backend_test.py +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -450,6 +450,21 @@ def test_invalid_log_data_is_logged_and_ignored(self, clean_databus, custom_dir, finally: backend.finalize(FLContext()) + def test_log_processing_error_is_logged_and_ignored(self, clean_databus, custom_dir, caplog): + executor = MagicMock() + executor.fire_log_analytics.side_effect = RuntimeError("analytics failed") + backend, _ = _initialized_backend(custom_dir, executor=executor) + try: + backend._log_result_callback( + TOPIC_LOG_DATA, + {"key": "accuracy", "value": 0.9, "data_type": AnalyticsDataType.SCALAR}, + clean_databus, + ) + + assert "failed to process trainer LOG data" in caplog.text + finally: + backend.finalize(FLContext()) + class TestMetaPassThrough: def test_meta_uses_raw_full_and_context_task_names(self, custom_dir): From 4a553d0ffbe1c948db35dbd950fbb15bcf999082 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 00:00:39 -0700 Subject: [PATCH 06/12] Add ScriptRunner execution_mode selecting the new ClientAPIExecutor (EX-4) ScriptRunner(script="client.py", execution_mode="in_process") wires the new ClientAPIExecutor instead of the legacy executor stack -- existing examples flip with one argument. Validated end-to-end: the hello-numpy example's unmodified client script ran a 3-round 2-client simulator FedAvg on the new executor with a mathematically correct aggregated model; the unit tests pin that add_to_fed_job produces exactly the executor/args that run validated. Contract: - Default None: legacy behavior byte-for-byte unchanged. - "in_process": new executor; the params boundary is pass-through (no ParamsConverters per FLARE-2698), so the framework format resolution -- including its torch/tensorflow import checks -- is skipped, and the legacy stack args (launch_external_process, executor, task_pipe, launcher, metric_relay, metric_pipe) are rejected as conflicts. - "external_process"/"attach": rejected at job BUILD time with a clear message until their backends land (no deferred START_RUN panic). --- nvflare/job_config/script_runner.py | 59 ++++++++++++++++++- .../job_config/script_runner_test.py | 55 +++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/nvflare/job_config/script_runner.py b/nvflare/job_config/script_runner.py index dc3b370b16..3fc583b87d 100644 --- a/nvflare/job_config/script_runner.py +++ b/nvflare/job_config/script_runner.py @@ -16,6 +16,7 @@ from nvflare.apis.fl_constant import SystemVarName from nvflare.app_common.abstract.launcher import Launcher +from nvflare.app_common.executors.client_api_executor import ALL_EXECUTION_MODES, ClientAPIExecutor, ExecutionMode from nvflare.app_common.executors.client_api_launcher_executor import ClientAPILauncherExecutor from nvflare.app_common.executors.in_process_client_api_executor import InProcessClientAPIExecutor from nvflare.app_common.launchers.subprocess_launcher import SubprocessLauncher @@ -64,6 +65,7 @@ def __init__( shutdown_timeout: float = 0.0, memory_gc_rounds: int = 0, cuda_empty_cache: bool = False, + execution_mode: Optional[str] = None, ): """BaseScriptRunner is used with FedJob API to run or launch a script. @@ -116,6 +118,16 @@ def __init__( shutdown_timeout (float): If provided, will wait for this number of seconds before shutdown. Only used if `launch_external_process` is True. Defaults to 0.0. + + execution_mode (Optional[str]): Selects the new ClientAPIExecutor + (design: docs/design/client_api_execution_modes.md) instead of the legacy + executor stack. Currently only "in_process" is available; "external_process" + and "attach" land with their backends. When set, the params boundary is + pass-through (no ParamsConverters per FLARE-2698): `framework`, + `server_expected_format` and `params_transfer_type` are not used, and the + legacy stack args (`launch_external_process`, `executor`, `task_pipe`, + `launcher`, `metric_relay`, `metric_pipe`) must not be set. Defaults to None + (legacy behavior, unchanged). """ self._script = script self._script_args = script_args @@ -128,9 +140,37 @@ def __init__( self._launch_once = launch_once self._shutdown_timeout = shutdown_timeout + if execution_mode is not None: + if execution_mode not in ALL_EXECUTION_MODES: + raise ValueError(f"invalid execution_mode '{execution_mode}': must be one of {ALL_EXECUTION_MODES}") + if execution_mode != ExecutionMode.IN_PROCESS: + raise ValueError( + f"execution_mode '{execution_mode}' is not yet available via ScriptRunner: " + f"only '{ExecutionMode.IN_PROCESS}' is supported until the corresponding backend lands" + ) + conflicts = { + "launch_external_process": launch_external_process or None, + "executor": executor, + "task_pipe": task_pipe, + "launcher": launcher, + "metric_relay": metric_relay, + "metric_pipe": metric_pipe, + } + set_conflicts = [name for name, value in conflicts.items() if value] + if set_conflicts: + raise ValueError( + f"execution_mode is mutually exclusive with the legacy executor stack args: {set_conflicts}" + ) + self._execution_mode = execution_mode + self._params_exchange_format = None - if self._framework == FrameworkType.PYTORCH: + # The new-executor path is pass-through (no ParamsConverters per FLARE-2698), so the + # framework format resolution -- including its torch/tensorflow import checks -- is + # skipped entirely when execution_mode is set. + if execution_mode is not None: + pass + elif self._framework == FrameworkType.PYTORCH: _, torch_ok = optional_import(module="torch") if torch_ok: self._params_exchange_format = ExchangeFormat.PYTORCH @@ -208,6 +248,18 @@ def add_to_fed_job(self, job: FedJob, ctx, **kwargs): tasks = kwargs.get("tasks", ["*"]) comp_ids = {} + if self._execution_mode is not None: + executor = ClientAPIExecutor( + execution_mode=self._execution_mode, + task_script_path=self._script, + task_script_args=self._script_args, + memory_gc_rounds=self._memory_gc_rounds, + cuda_empty_cache=self._cuda_empty_cache, + ) + job.add_executor(executor, tasks=tasks, ctx=ctx) + job.add_resources(resources=[self._script], ctx=ctx) + return comp_ids + if self._launch_external_process: task_pipe = self._task_pipe if self._task_pipe else self._create_cell_pipe() task_pipe_id = job.add_component("pipe", task_pipe, ctx) @@ -320,6 +372,7 @@ def __init__( shutdown_timeout: float = 0.0, memory_gc_rounds: int = 0, cuda_empty_cache: bool = False, + execution_mode: Optional[str] = None, ): """ScriptRunner is used with FedJob API to run or launch a script. @@ -343,6 +396,9 @@ def __init__( or on each task. Only used if `launch_external_process` is True. Defaults to True. shutdown_timeout (float): If provided, will wait for this number of seconds before shutdown. Only used if `launch_external_process` is True. Defaults to 0.0. + execution_mode (Optional[str]): Selects the new ClientAPIExecutor instead of the + legacy executor stack; only "in_process" is currently available. See + BaseScriptRunner for the full contract. Defaults to None (legacy behavior). """ super().__init__( script=script, @@ -358,4 +414,5 @@ def __init__( shutdown_timeout=shutdown_timeout, memory_gc_rounds=memory_gc_rounds, cuda_empty_cache=cuda_empty_cache, + execution_mode=execution_mode, ) diff --git a/tests/unit_test/job_config/script_runner_test.py b/tests/unit_test/job_config/script_runner_test.py index eb71b17cf3..b186364d92 100644 --- a/tests/unit_test/job_config/script_runner_test.py +++ b/tests/unit_test/job_config/script_runner_test.py @@ -301,3 +301,58 @@ def test_memory_parameters_with_different_frameworks(self, framework): assert runner._memory_gc_rounds == 1 assert runner._cuda_empty_cache is True assert runner._framework == framework + + +class TestExecutionModeSelection: + """execution_mode selects the new ClientAPIExecutor (design: client_api_execution_modes.md).""" + + def test_in_process_mode_constructs_without_framework_imports(self): + # the new path is pass-through (no ParamsConverters), so the default + # framework=PYTORCH must NOT trigger the torch import check + runner = ScriptRunner(script="train.py", execution_mode="in_process") + assert runner._execution_mode == "in_process" + assert runner._params_exchange_format is None + + def test_invalid_mode_rejected(self): + with pytest.raises(ValueError, match="invalid execution_mode"): + ScriptRunner(script="train.py", execution_mode="bogus") + + @pytest.mark.parametrize("mode", ["external_process", "attach"]) + def test_not_yet_available_modes_fail_at_build_time(self, mode): + # fail when the job is BUILT, not when the backend panics at START_RUN + with pytest.raises(ValueError, match="not yet available"): + ScriptRunner(script="train.py", execution_mode=mode) + + def test_conflicts_with_legacy_stack_args(self): + with pytest.raises(ValueError, match="mutually exclusive.*launch_external_process"): + ScriptRunner(script="train.py", execution_mode="in_process", launch_external_process=True) + + def test_default_none_preserves_legacy_behavior(self): + runner = ScriptRunner(script="train.py", script_args="--epochs 10", framework=FrameworkType.NUMPY) + assert runner._execution_mode is None + + def test_add_to_fed_job_wires_client_api_executor(self): + from nvflare.app_common.executors.client_api_executor import ClientAPIExecutor + from nvflare.job_config.api import FedJob + + with patch("os.path.isfile", return_value=True), patch("os.path.exists", return_value=True): + job = FedJob(name="smoke") + runner = ScriptRunner( + script="client.py", script_args="--update_type full", execution_mode="in_process", memory_gc_rounds=2 + ) + job.to(runner, "site-1", tasks=["train"]) + + app = job._deploy_map["site-1"] + executors = app.app_config.executors + assert len(executors) == 1 + executor = executors[0].executor + assert isinstance(executor, ClientAPIExecutor) + # the exact args the simulator smoke test validated end-to-end + assert executor._execution_mode == "in_process" + assert executor._task_script_path == "client.py" + assert executor._task_script_args == "--update_type full" + assert executor._memory_gc_rounds == 2 + # no legacy stack components (pipes/launcher/relay) were added + assert app.app_config.components == {} + # the script rides along as an app resource + assert "client.py" in app.app_config.ext_scripts From 4c4598f57fee6f1bd77f21b40115fee602825502 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 00:05:16 -0700 Subject: [PATCH 07/12] Raise ClientAPIExecutor patch coverage --- .../executors/client_api/backend_spec.py | 4 -- .../executors/client_api_executor.py | 5 +- .../executors/client_api_executor_test.py | 26 ++++++++ .../executors/in_process_backend_test.py | 66 +++++++++++++++++++ tests/unit_test/client/in_process/api_test.py | 12 ++++ 5 files changed, 107 insertions(+), 6 deletions(-) diff --git a/nvflare/app_common/executors/client_api/backend_spec.py b/nvflare/app_common/executors/client_api/backend_spec.py index 8874b4e1f2..612b2c4c16 100644 --- a/nvflare/app_common/executors/client_api/backend_spec.py +++ b/nvflare/app_common/executors/client_api/backend_spec.py @@ -135,7 +135,6 @@ def initialize(self, context: ClientAPIBackendContext, fl_ctx: FLContext) -> Non context: the frozen backend configuration and executor back-reference. fl_ctx: the FLContext of the START_RUN event. """ - pass @abstractmethod def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort_signal: Signal) -> Shareable: @@ -162,7 +161,6 @@ def execute(self, task_name: str, shareable: Shareable, fl_ctx: FLContext, abort Returns: The result Shareable (an error reply on failure/abort - never None). """ - pass @abstractmethod def handle_event(self, event_type: str, fl_ctx: FLContext) -> None: @@ -177,7 +175,6 @@ def handle_event(self, event_type: str, fl_ctx: FLContext) -> None: event_type: the fired event type. fl_ctx: the FLContext of the event. """ - pass @abstractmethod def finalize(self, fl_ctx: FLContext) -> None: @@ -194,4 +191,3 @@ def finalize(self, fl_ctx: FLContext) -> None: Args: fl_ctx: the FLContext of the END_RUN event. """ - pass diff --git a/nvflare/app_common/executors/client_api_executor.py b/nvflare/app_common/executors/client_api_executor.py index ae0d5f37e7..136f39c8a8 100644 --- a/nvflare/app_common/executors/client_api_executor.py +++ b/nvflare/app_common/executors/client_api_executor.py @@ -444,13 +444,14 @@ def _backend_registry(self) -> Dict[str, Callable[[], ClientAPIBackendSpec]]: } def _create_backend(self) -> ClientAPIBackendSpec: - factory = self._backend_registry().get(self._execution_mode) + registry = self._backend_registry() + factory = registry.get(self._execution_mode) if factory is None: # Unreachable via the public constructor (execution_mode is validated there); # guards subclasses that override _backend_registry(). raise ValueError( f"no backend factory registered for execution_mode '{self._execution_mode}': " - f"registered modes are {list(self._backend_registry().keys())}" + f"registered modes are {list(registry.keys())}" ) return factory() diff --git a/tests/unit_test/app_common/executors/client_api_executor_test.py b/tests/unit_test/app_common/executors/client_api_executor_test.py index 792a5f382a..a0749620e4 100644 --- a/tests/unit_test/app_common/executors/client_api_executor_test.py +++ b/tests/unit_test/app_common/executors/client_api_executor_test.py @@ -422,6 +422,14 @@ def test_other_events_forwarded_to_backend(self): executor.handle_event(EventType.ABOUT_TO_END_RUN, fl_ctx) assert ("handle_event", EventType.ABOUT_TO_END_RUN) in backend.calls + def test_backend_event_exception_is_logged_and_ignored(self): + executor, backend, fl_ctx, _ = self._make_started_executor() + backend.handle_event = Mock(side_effect=RuntimeError("event failed")) + + executor.handle_event(EventType.ABOUT_TO_END_RUN, fl_ctx) + + assert executor._backend is backend + def test_end_run_finalizes_and_clears_backend(self): executor, backend, fl_ctx, _ = self._make_started_executor() executor.handle_event(EventType.END_RUN, fl_ctx) @@ -429,6 +437,14 @@ def test_end_run_finalizes_and_clears_backend(self): reply = executor.execute("train", Shareable(), fl_ctx, Signal()) assert reply.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + def test_backend_finalize_exception_is_logged_and_ignored(self): + executor, backend, fl_ctx, _ = self._make_started_executor() + backend.finalize = Mock(side_effect=RuntimeError("finalize failed")) + + executor.handle_event(EventType.END_RUN, fl_ctx) + + assert executor._backend is None + def test_backend_exception_in_execute_becomes_error_reply(self): executor, backend, fl_ctx, _ = self._make_started_executor() backend.execute = Mock(side_effect=RuntimeError("boom")) @@ -490,6 +506,16 @@ def test_backend_spec_is_abstract(self): with pytest.raises(TypeError): ClientAPIBackendSpec() + def test_missing_backend_factory_reports_registered_modes(self, monkeypatch): + executor = ClientAPIExecutor(execution_mode="in_process") + registry = Mock(return_value={}) + monkeypatch.setattr(executor, "_backend_registry", registry) + + with pytest.raises(ValueError, match=r"registered modes are \[\]"): + executor._create_backend() + + registry.assert_called_once_with() + class TestAnalyticsOwnership: """fire_log_analytics is the executor-owned LOG-to-analytics conversion surface.""" diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py index f17ba5b690..0ee73dee72 100644 --- a/tests/unit_test/app_common/executors/in_process_backend_test.py +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -199,6 +199,45 @@ def test_finalize_idempotent_and_cleans_databus(self, clean_databus, custom_dir) assert not _backend_callbacks_subscribed(clean_databus, backend) assert not backend._task_fn_thread.is_alive() + def test_initialize_configures_memory_management(self, custom_dir): + backend, _ = _initialized_backend(custom_dir, memory_gc_rounds=3, cuda_empty_cache=True) + try: + assert backend._client_api._memory_gc_rounds == 3 + assert backend._client_api._cuda_empty_cache is True + finally: + backend.finalize(FLContext()) + + def test_finalize_logs_stop_publish_failure(self, exited_custom_dir, caplog): + backend, _ = _initialized_backend(exited_custom_dir) + backend._task_fn_thread.join(timeout=1.0) + backend._event_manager.fire_event = Mock(side_effect=RuntimeError("stop failed")) + + backend.finalize(FLContext()) + + assert "stop failed" in caplog.text + + def test_finalize_logs_thread_join_failure(self, exited_custom_dir, caplog): + backend, _ = _initialized_backend(exited_custom_dir) + backend._task_fn_thread.join(timeout=1.0) + thread = Mock() + thread.is_alive.return_value = True + thread.join.side_effect = RuntimeError("join failed") + backend._task_fn_thread = thread + + backend.finalize(FLContext()) + + assert "join failed" in caplog.text + + def test_unwind_logs_cleanup_failure(self, caplog): + backend = InProcessBackend() + backend._data_bus = Mock() + backend._subscribed = True + backend._data_bus.unsubscribe.side_effect = RuntimeError("unsubscribe failed") + + backend._unwind() + + assert "unsubscribe failed" in caplog.text + class TestExecute: def test_execute_round_trip(self, clean_databus, custom_dir): @@ -275,6 +314,17 @@ def test_execute_fails_fast_when_trainer_thread_exits(self, clean_databus, exite finally: backend.finalize(FLContext()) + def test_execute_detects_trainer_exit_while_waiting(self, clean_databus, custom_dir, monkeypatch): + backend, fl_ctx = _initialized_backend(custom_dir, result_wait_timeout=2.0) + monkeypatch.setattr(backend, "_trainer_thread_is_alive", Mock(side_effect=[True, False])) + try: + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + + assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + assert backend._abort_reason == "trainer thread exited before producing a result" + finally: + backend.finalize(FLContext()) + def test_multi_round_sequential_execute(self, clean_databus, custom_dir): """The same backend serves consecutive rounds (the legacy executor's normal life).""" backend, fl_ctx = _initialized_backend(custom_dir) @@ -363,6 +413,22 @@ def test_unsafe_job_error_propagates(self, clean_databus, custom_dir, monkeypatc finally: backend.finalize(FLContext()) + def test_execute_exception_returns_execution_exception(self, clean_databus, custom_dir, monkeypatch): + backend, fl_ctx = _initialized_backend(custom_dir) + try: + monkeypatch.setattr(backend, "_prepare_task_meta", Mock(side_effect=RuntimeError("boom"))) + + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + + assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + assert backend._abort_reason is not None + finally: + backend.finalize(FLContext()) + + def test_handle_event_is_noop(self): + backend = InProcessBackend() + backend.handle_event("custom_event", FLContext()) + class TestMultiBackend: def test_finalizing_backend_does_not_clear_successors_client_api(self, clean_databus, custom_dir): diff --git a/tests/unit_test/client/in_process/api_test.py b/tests/unit_test/client/in_process/api_test.py index e8fed4bc9c..7faff5ab12 100644 --- a/tests/unit_test/client/in_process/api_test.py +++ b/tests/unit_test/client/in_process/api_test.py @@ -93,6 +93,18 @@ def test_init_subscriptions2(self): TOPIC_STOP, ] + def test_close_unsubscribes_api_callbacks(self): + data_bus = DataBus() + data_bus.subscribers.clear() + client_api = InProcessClientAPI(self.task_metadata) + + client_api.close() + client_api.close() # idempotent + + assert TOPIC_GLOBAL_RESULT not in data_bus.subscribers + assert TOPIC_ABORT not in data_bus.subscribers + assert TOPIC_STOP not in data_bus.subscribers + def test_memory_management_defaults(self): """Test that memory management is disabled by default.""" client_api = InProcessClientAPI(self.task_metadata) From 171b6c1d1131e67e263488c33fdf4eb143cb15e7 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 12:14:21 -0700 Subject: [PATCH 08/12] Bound trainer teardown so a wedged trainer cannot hang CJ exit finalize() joined the trainer thread unbounded. Safe in the legacy executor (execute() never returns while the trainer lives), but result_wait_timeout makes a live-trainer finalize reachable: a trainer wedged in user code never observes TOPIC_STOP (it is only seen at the next flare call), and the join would hang CJ/simulator teardown forever. finalize() now joins with a 30s bound and abandons with a loud log; the trainer thread is daemon (deliberate divergence from legacy, documented) so an abandoned trainer cannot block process exit either. Regression test drives a genuinely stuck trainer. --- .../client_api/in_process_backend.py | 27 ++++++++++++++++--- .../executors/in_process_backend_test.py | 22 ++++++++++++++- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/nvflare/app_common/executors/client_api/in_process_backend.py b/nvflare/app_common/executors/client_api/in_process_backend.py index 935159528e..4d332a3676 100644 --- a/nvflare/app_common/executors/client_api/in_process_backend.py +++ b/nvflare/app_common/executors/client_api/in_process_backend.py @@ -70,6 +70,13 @@ # The legacy executor exposed this as result_pull_interval (default 0.5); the frozen # surface deliberately drops the knob, so the default becomes the behavior. _RESULT_POLL_INTERVAL = 0.5 + +# Bound for finalize()'s trainer-thread join. TOPIC_STOP is only observed at the trainer's +# next flare call; a trainer stuck in user code (a long GPU op, a loop that never checks +# flare.is_running()) may never observe it. With result_wait_timeout, execute() can return +# while the trainer is still alive, so an unbounded join here could hang CJ/simulator +# teardown forever. +_TRAINER_STOP_JOIN_TIMEOUT = 30.0 _NO_RESULT = object() @@ -127,9 +134,14 @@ def initialize(self, context: ClientAPIBackendContext, fl_ctx: FLContext) -> Non # this is how the trainer script's flare.init() finds the API instance self._data_bus.put_data(CLIENT_API_KEY, self._client_api) - # non-daemon, matching the legacy executor: process exit waits for the trainer, - # and finalize() joins it after TOPIC_STOP - self._task_fn_thread = threading.Thread(target=task_fn_wrapper.run, name="client_api_in_process_trainer") + # daemon: a deliberate divergence from the legacy executor (non-daemon). With + # result_wait_timeout, execute() can return while the trainer is still running; + # a trainer wedged in user code never observes TOPIC_STOP, and a non-daemon + # thread would then block process exit even after finalize() gives up its + # bounded join. finalize() still joins cooperatively first. + self._task_fn_thread = threading.Thread( + target=task_fn_wrapper.run, name="client_api_in_process_trainer", daemon=True + ) self._task_fn_thread.start() except Exception: # contract (backend_spec): initialize() self-unwinds its partial setup on failure; @@ -254,7 +266,14 @@ def finalize(self, fl_ctx: FLContext) -> None: try: thread = self._task_fn_thread if thread is not None and thread.is_alive(): - thread.join() + # bounded: TOPIC_STOP is only seen at the trainer's next flare call, and a + # trainer stuck in user code may never make one -- do not hang teardown on it + thread.join(timeout=_TRAINER_STOP_JOIN_TIMEOUT) + if thread.is_alive(): + self.logger.error( + f"in-process trainer thread did not stop within {_TRAINER_STOP_JOIN_TIMEOUT}s " + f"after TOPIC_STOP; abandoning it (daemon thread, will not block process exit)" + ) except Exception: self.logger.error(secure_format_traceback()) finally: diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py index 0ee73dee72..8b82b16251 100644 --- a/tests/unit_test/app_common/executors/in_process_backend_test.py +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -23,7 +23,7 @@ """ import time -from unittest.mock import MagicMock, Mock +from unittest.mock import MagicMock, Mock, patch import pytest @@ -36,6 +36,7 @@ from nvflare.apis.signal import Signal from nvflare.app_common.app_constant import AppConstants from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext +from nvflare.app_common.executors.client_api import in_process_backend as ipb_module from nvflare.app_common.executors.client_api.in_process_backend import InProcessBackend from nvflare.app_common.executors.client_api_executor import ClientAPIExecutor from nvflare.client.api_spec import CLIENT_API_KEY @@ -199,6 +200,25 @@ def test_finalize_idempotent_and_cleans_databus(self, clean_databus, custom_dir) assert not _backend_callbacks_subscribed(clean_databus, backend) assert not backend._task_fn_thread.is_alive() + def test_finalize_bounded_when_trainer_stuck_in_user_code(self, tmp_path, caplog): + """A trainer wedged in user code never observes TOPIC_STOP: with result_wait_timeout, + execute() returns while the thread still runs, so finalize() must join with a bound + and abandon (daemon thread) instead of hanging CJ/simulator teardown forever.""" + (tmp_path / "train.py").write_text("import time\nwhile True: time.sleep(0.5)\n") + backend, fl_ctx = _initialized_backend(str(tmp_path), result_wait_timeout=0.05) + try: + result = backend.execute("train", Shareable(), fl_ctx, Signal()) + assert result.get_return_code() == ReturnCode.EXECUTION_EXCEPTION + assert backend._task_fn_thread.is_alive() # the reachable-hang precondition + assert backend._task_fn_thread.daemon # a wedged trainer cannot block process exit + finally: + with patch.object(ipb_module, "_TRAINER_STOP_JOIN_TIMEOUT", 0.5): + start = time.monotonic() + backend.finalize(FLContext()) + elapsed = time.monotonic() - start + assert elapsed < 5.0, "finalize must not hang on a stuck trainer" + assert "did not stop within" in caplog.text + def test_initialize_configures_memory_management(self, custom_dir): backend, _ = _initialized_backend(custom_dir, memory_gc_rounds=3, cuda_empty_cache=True) try: From 9d53b3dca45bfc84796cc2e793695daafd19b765 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 13:31:43 -0700 Subject: [PATCH 09/12] Fix import ordering in in_process_backend_test --- tests/unit_test/app_common/executors/in_process_backend_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py index 8b82b16251..0eb6147690 100644 --- a/tests/unit_test/app_common/executors/in_process_backend_test.py +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -35,8 +35,8 @@ from nvflare.apis.shareable import Shareable from nvflare.apis.signal import Signal from nvflare.app_common.app_constant import AppConstants -from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext from nvflare.app_common.executors.client_api import in_process_backend as ipb_module +from nvflare.app_common.executors.client_api.backend_spec import ClientAPIBackendContext from nvflare.app_common.executors.client_api.in_process_backend import InProcessBackend from nvflare.app_common.executors.client_api_executor import ClientAPIExecutor from nvflare.client.api_spec import CLIENT_API_KEY From ad3ac601f42ea47d0713604c2e2079e19b21cd65 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 16:46:11 -0700 Subject: [PATCH 10/12] Remove process/history notes from user-facing docstrings Module and constructor docs now describe the API as it is, not how it got here: dropped the interface-freeze/plan-id/follow-up-PR narration (some of it already stale -- in_process ships in this PR, not a follow-up), the internal ticket references, and the design-divergence changelog section. Kept the user-relevant substance: mode availability, and why the surface has no parameter-conversion args (pass-through boundary; conversion belongs to client-edge filters; FULL/DIFF is a model-registry concern). --- .../executors/client_api/backend_spec.py | 15 ++++--- .../client_api/in_process_backend.py | 13 +++--- .../executors/client_api_executor.py | 42 ++++++------------- nvflare/job_config/script_runner.py | 4 +- 4 files changed, 27 insertions(+), 47 deletions(-) diff --git a/nvflare/app_common/executors/client_api/backend_spec.py b/nvflare/app_common/executors/client_api/backend_spec.py index 612b2c4c16..717e409e02 100644 --- a/nvflare/app_common/executors/client_api/backend_spec.py +++ b/nvflare/app_common/executors/client_api/backend_spec.py @@ -17,9 +17,9 @@ Design: docs/design/client_api_execution_modes.md ("Overview", "Execution Modes", "Client API Backends"). One ClientAPIExecutor delegates to one mode-specific backend: -- in_process: trainer runs inside the Client Job (CJ) process over DataBus (EX-3) -- external_process: NVFlare launches and owns the trainer process tree over Cell (EP-4) -- attach: an externally owned trainer attaches over Cell (AT-2) +- in_process: trainer runs inside the Client Job (CJ) process over DataBus +- external_process: NVFlare launches and owns the trainer process tree over Cell +- attach: an externally owned trainer attaches over Cell This module is internal to NVFlare. It is not a user extension point; users configure ``ClientAPIExecutor(execution_mode=...)`` only. @@ -78,11 +78,10 @@ class ClientAPIBackendContext: task_wait_timeout: Optional[float] = None result_wait_timeout: Optional[float] = None # NOTE: params_exchange_format / params_transfer_type / server_expected_format and the - # from/to_nvflare_converter ids are intentionally NOT here. Per FLARE-2698, param - # conversion between the framework-agnostic aggregation representation (numpy) and the - # framework-native training representation moves out of the executor to send/receive - # filters at the client edge; the Client API boundary is pass-through. Transfer type - # (FULL/DIFF) stays a Client API concern (model_registry), decided separately. + # from/to_nvflare_converter ids are intentionally NOT here: the Client API boundary is + # pass-through, and format conversion between the aggregation representation and the + # framework-native training representation belongs to send/receive filters at the client + # edge. Transfer type (FULL/DIFF) is a model-registry concern. # task-name / rank contract (all modes) train_task_name: str = AppConstants.TASK_TRAIN evaluate_task_name: str = AppConstants.TASK_VALIDATION diff --git a/nvflare/app_common/executors/client_api/in_process_backend.py b/nvflare/app_common/executors/client_api/in_process_backend.py index 4d332a3676..e010e0d905 100644 --- a/nvflare/app_common/executors/client_api/in_process_backend.py +++ b/nvflare/app_common/executors/client_api/in_process_backend.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""in_process backend for ClientAPIExecutor (plan: EX-3). +"""in_process backend for ClientAPIExecutor. Ports InProcessClientAPIExecutor's DataBus machinery behind the frozen ClientAPIBackendSpec surface, with the behavior-parity bar "nothing user-visible": @@ -20,10 +20,9 @@ InProcessClientAPI via the DataBus CLIENT_API_KEY entry, receives tasks over TOPIC_GLOBAL_RESULT, and returns results over TOPIC_LOCAL_RESULT. -Differences from the legacy executor, all mandated by the design -(docs/design/client_api_execution_modes.md) and the backend contract: +Differences from the legacy executor (see docs/design/client_api_execution_modes.md): -- No ParamsConverters and no exchange-format/transfer-type knobs (FLARE-2698): +- No ParamsConverters and no exchange-format/transfer-type knobs: the Client API boundary passes params through unconverted (ExchangeFormat.RAW) and V1 sends full params (TransferType.FULL); DIFF support returns with the model_registry transfer-type decision, and format conversion moves to @@ -309,9 +308,9 @@ def _prepare_task_meta(self, fl_ctx: FLContext, task_name: Optional[str]) -> dic ConfigKey.TASK_NAME: task_name, ConfigKey.TASK_EXCHANGE: { ConfigKey.TRAIN_WITH_EVAL: context.train_with_evaluation, - # FLARE-2698: the Client API boundary passes params through unconverted; format - # conversion happens in send/receive filters at the client edge, and DIFF returns - # with the model_registry transfer-type decision + # the Client API boundary passes params through unconverted; format conversion + # happens in send/receive filters at the client edge, and DIFF returns with the + # model-registry transfer-type decision ConfigKey.EXCHANGE_FORMAT: ExchangeFormat.RAW, ConfigKey.TRANSFER_TYPE: TransferType.FULL, ConfigKey.TRAIN_TASK_NAME: context.train_task_name, diff --git a/nvflare/app_common/executors/client_api_executor.py b/nvflare/app_common/executors/client_api_executor.py index 136f39c8a8..e2c14e6f73 100644 --- a/nvflare/app_common/executors/client_api_executor.py +++ b/nvflare/app_common/executors/client_api_executor.py @@ -18,30 +18,15 @@ "Execution Modes", "Configuration Surface"). This module path is normative - job configs reference ``nvflare.app_common.executors.client_api_executor.ClientAPIExecutor``. -This is the interface-freeze skeleton (plan: EX-2). The constructor surface below is frozen; -the mode backends land in follow-up PRs (in_process, external_process, attach). - -Divergence from the design's V1 "Configuration Surface" list ------------------------------------------------------------- -The design's V1 arg list is a subset; the frozen surface below adds the following load-bearing -args that the mode backends require and that both legacy executors -(InProcessClientAPIExecutor / ClientAPILauncherExecutor) already expose. They are recorded here -so the design's Configuration Surface can be synced separately: - -- ``task_script_path`` / ``task_script_args`` - in_process script entry point (the in_process - backend runs a user script via TaskScriptRunner; ``command`` names the external_process - trainer, ``task_script_path`` names the in_process one). -- ``train_task_name`` / ``evaluate_task_name`` / ``submit_model_task_name`` / - ``train_with_evaluation`` - power the rank-contract APIs flare.is_train()/is_evaluate()/ - is_submit_model(). -- ``memory_gc_rounds`` / ``cuda_empty_cache`` - periodic GC / CUDA cache management - -Deliberately excluded (per FLARE-2698): ``params_exchange_format`` / ``params_transfer_type`` / -``server_expected_format`` / ``from_nvflare_converter_id`` / ``to_nvflare_converter_id``. Param -conversion moves from executor-owned ParamsConverters to send/receive filters at the client -edge (the intermediate layers pass through), so these are not frozen into this surface. The -transfer type (FULL/DIFF) remains a Client API concern (model_registry) and is decided -separately from the converter removal. +Availability: ``in_process`` is fully supported. ``external_process`` and ``attach`` are +not yet implemented; selecting them fails cleanly at job startup. + +Unlike the legacy executors (InProcessClientAPIExecutor / ClientAPILauncherExecutor), this +surface has no parameter-conversion args (``params_exchange_format`` / +``params_transfer_type`` / ``server_expected_format`` / converter ids): the Client API +boundary passes parameters through unconverted, and format conversion belongs to +send/receive filters at the client edge. Transfer type (FULL/DIFF) is a model-registry +concern, configured elsewhere. """ from typing import Callable, Dict, Optional @@ -122,9 +107,8 @@ def __init__( ): """Initializes the ClientAPIExecutor. - This constructor surface is frozen (design: "Configuration Surface"); parameter renames - break the surface-freeze test and downstream backend PRs. See the module docstring for the - args added beyond the design's V1 Configuration Surface list. + Parameter names are part of the public job-config surface: renames are breaking + changes (guarded by the surface-freeze test). Args: execution_mode (str): One of "in_process", "external_process", or "attach". Required. @@ -267,7 +251,7 @@ def __init__( # ConvertToFedEvent widget (added by BaseFedJob) to re-fire it as # "fed.analytix_log_stats" - today's in-process executor behavior. # True: fire a federation-scoped event directly (today's MetricRelay behavior for - # ex-process). Cell backends (EP-4/AT-2) may select this path in initialize(). + # ex-process). Cell backends (external_process/attach) may select this path in initialize(). self._analytics_fire_fed_event: bool = False def handle_event(self, event_type: str, fl_ctx: FLContext): @@ -463,9 +447,7 @@ def _create_in_process_backend(self) -> ClientAPIBackendSpec: return InProcessBackend() def _create_external_process_backend(self) -> ClientAPIBackendSpec: - # Implemented in a follow-up PR (plan: EP-4). raise NotImplementedError("external_process execution mode is not yet implemented in this release") def _create_attach_backend(self) -> ClientAPIBackendSpec: - # Implemented in a follow-up PR (plan: AT-2). raise NotImplementedError("attach execution mode is not yet implemented in this release") diff --git a/nvflare/job_config/script_runner.py b/nvflare/job_config/script_runner.py index 3fc583b87d..a29f5daf4d 100644 --- a/nvflare/job_config/script_runner.py +++ b/nvflare/job_config/script_runner.py @@ -123,7 +123,7 @@ def __init__( (design: docs/design/client_api_execution_modes.md) instead of the legacy executor stack. Currently only "in_process" is available; "external_process" and "attach" land with their backends. When set, the params boundary is - pass-through (no ParamsConverters per FLARE-2698): `framework`, + pass-through (no ParamsConverters): `framework`, `server_expected_format` and `params_transfer_type` are not used, and the legacy stack args (`launch_external_process`, `executor`, `task_pipe`, `launcher`, `metric_relay`, `metric_pipe`) must not be set. Defaults to None @@ -165,7 +165,7 @@ def __init__( self._params_exchange_format = None - # The new-executor path is pass-through (no ParamsConverters per FLARE-2698), so the + # The new-executor path is pass-through (no ParamsConverters), so the # framework format resolution -- including its torch/tensorflow import checks -- is # skipped entirely when execution_mode is set. if execution_mode is not None: From 247b0592d672eb8eb899ad4a57e3057800c2f709 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 17:51:15 -0700 Subject: [PATCH 11/12] Gate outgoing publications from a closed InProcessClientAPI finalize() can abandon a still-running daemon trainer after its bounded join; that thread still holds the API object and may resume later. close() detached only the incoming subscriptions -- the zombie's send()/log() could still publish onto the singleton DataBus, where a successor job's backend is now subscribed, feeding it a dead job's result or metric (results carry no job correlation). close() now also marks the API closed; send()/log() DROP with a warning and receive() returns None immediately. Dropping -- not raising -- is deliberate: an exception would propagate into TaskScriptRunner's catch-all, which fires TOPIC_ABORT onto the same singleton bus and would poison the successor, the exact cross-job leak being prevented. Tests cover the dropped send/log, the fast-exit receive, and that an open API still publishes (no over-drop). --- .../client_api/in_process_backend.py | 3 +- nvflare/client/in_process/api.py | 40 +++++++++++++--- .../executors/in_process_backend_test.py | 47 +++++++++++++++++++ 3 files changed, 82 insertions(+), 8 deletions(-) diff --git a/nvflare/app_common/executors/client_api/in_process_backend.py b/nvflare/app_common/executors/client_api/in_process_backend.py index e010e0d905..5fb07d264f 100644 --- a/nvflare/app_common/executors/client_api/in_process_backend.py +++ b/nvflare/app_common/executors/client_api/in_process_backend.py @@ -271,7 +271,8 @@ def finalize(self, fl_ctx: FLContext) -> None: if thread.is_alive(): self.logger.error( f"in-process trainer thread did not stop within {_TRAINER_STOP_JOIN_TIMEOUT}s " - f"after TOPIC_STOP; abandoning it (daemon thread, will not block process exit)" + f"after TOPIC_STOP; abandoning it (daemon thread, will not block process exit; " + f"its API is closed, so any later send/log from it is dropped)" ) except Exception: self.logger.error(secure_format_traceback()) diff --git a/nvflare/client/in_process/api.py b/nvflare/client/in_process/api.py index dccef79827..95f24b709f 100644 --- a/nvflare/client/in_process/api.py +++ b/nvflare/client/in_process/api.py @@ -63,6 +63,7 @@ def __init__(self, task_metadata: dict, result_check_interval: float = 2.0): self.stop_reason = "" self.abort = False self.stop = False + self.closed = False self.rank = None self.receive_called = False # to check if users have call received for a new model @@ -113,7 +114,25 @@ def configure_memory_management(self, gc_rounds: int = 0, cuda_empty_cache: bool if gc_rounds > 0: self.logger.info(f"Memory management enabled: cleanup every {gc_rounds} round(s)") + def _dropped_because_closed(self, what: str) -> bool: + """Outgoing-publication gate for a closed API. + + After close(), this instance belongs to a job that has ended, but an abandoned + trainer thread may still hold it and resume later. Its publications must not land + on the singleton DataBus where a successor job's backend is now subscribed. The + gate DROPS (with a warning) instead of raising: an exception would propagate into + TaskScriptRunner's catch-all, which fires TOPIC_ABORT onto the same singleton bus + and would poison the successor -- the exact cross-job leak this gate prevents. + """ + if not self.closed: + return False + self.logger.warning(f"dropping {what}: this Client API is closed (its job has ended)") + return True + def receive(self, timeout: Optional[float] = None) -> Optional[FLModel]: + if self.closed: + # closed API: behave as stopped -- the trainer's is_running()/receive loop exits + return None result = self.__receive(timeout) if result is not None: self.receive_called = True @@ -146,6 +165,8 @@ def __receive(self, timeout: Optional[float] = None) -> Optional[FLModel]: return self.fl_model def send(self, model: FLModel, clear_cache: bool = True) -> None: + if self._dropped_because_closed("result send"): + return if self.__continue_job(): self.logger.info("Try to send local model back to peer ") @@ -221,6 +242,8 @@ def is_submit_model(self) -> bool: return self.meta.get(ConfigKey.TASK_NAME) == self.client_config.get_submit_model_task() def log(self, key: str, value: Any, data_type: AnalyticsDataType, **kwargs): + if self._dropped_because_closed(f"metric log '{key}'"): + return if self.rank != "0": raise RuntimeError("only rank 0 can call log!") msg = dict(key=key, value=value, data_type=data_type, **kwargs) @@ -282,14 +305,17 @@ def shutdown(self): self.stop_reason = "API shutdown called." def close(self): - """Unsubscribes this API instance's DataBus callbacks. - - The DataBus is a process singleton: without this, a finished job's API instance - stays subscribed to TOPIC_GLOBAL_RESULT for the process lifetime, so every later - job's task publish also lands on the dead instance and pins its latest global - model in memory. Called by the owning executor/backend at teardown; safe to call - more than once (unsubscribe of an absent callback is a no-op). + """Detaches this API instance from the singleton DataBus, in both directions. + + Incoming: unsubscribes its callbacks -- without this, a finished job's API stays + subscribed to TOPIC_GLOBAL_RESULT for the process lifetime, so every later job's + task publish also lands on the dead instance and pins its latest global model. + Outgoing: marks the instance closed so send()/log() DROP instead of publishing -- + an abandoned trainer thread that resumes after its job ended must not feed results + or metrics to a successor job's backend. Called by the owning executor/backend at + teardown; idempotent. """ + self.closed = True self.data_bus.unsubscribe(TOPIC_GLOBAL_RESULT, self.__receive_callback) self.data_bus.unsubscribe(TOPIC_ABORT, self.__ask_to_abort) self.data_bus.unsubscribe(TOPIC_STOP, self.__ask_to_abort) diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py index 0eb6147690..5c625ff47f 100644 --- a/tests/unit_test/app_common/executors/in_process_backend_test.py +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -508,6 +508,53 @@ def raising_init(self, *args, **kwargs): assert not isinstance(getattr(cb, "__self__", None), InProcessClientAPI), f"leak on {topic}" +class TestClosedApiOutgoingGate: + """A closed API must not publish onto the singleton bus (zombie-trainer containment). + + finalize() can abandon a still-running daemon trainer after the bounded join; that + thread still holds the API object and may resume later. Its send()/log() must DROP -- + not publish (a successor job's backend is now subscribed to the same topics), and not + raise (an exception would hit TaskScriptRunner's catch-all, which fires TOPIC_ABORT + onto the singleton bus and would poison the successor).""" + + def test_finalize_closes_api_and_late_send_log_are_dropped(self, clean_databus, custom_dir): + backend, _ = _initialized_backend(custom_dir) + api = backend._client_api + backend.finalize(FLContext()) + assert api.closed is True + + published = [] + clean_databus.subscribe([TOPIC_LOCAL_RESULT, TOPIC_LOG_DATA], lambda t, d, b: published.append((t, d))) + + # the zombie resumes: neither call may publish or raise + api.send(None) + api.log("accuracy", 0.99, "SCALAR") + + assert published == [] + + def test_closed_receive_returns_none_fast(self, clean_databus, custom_dir): + backend, _ = _initialized_backend(custom_dir) + api = backend._client_api + backend.finalize(FLContext()) + + start = time.monotonic() + assert api.receive(timeout=10.0) is None + assert time.monotonic() - start < 1.0, "closed receive must not wait out its timeout" + + def test_open_api_still_publishes(self, clean_databus, custom_dir): + # the gate must not over-drop: an open API's log still lands on the bus + backend, _ = _initialized_backend(custom_dir) + try: + api = backend._client_api + api.rank = "0" + published = [] + clean_databus.subscribe([TOPIC_LOG_DATA], lambda t, d, b: published.append(d)) + api.log("accuracy", 0.99, "SCALAR") + assert len(published) == 1 + finally: + backend.finalize(FLContext()) + + class TestLogRouting: def test_log_data_routes_through_executor_fire_log_analytics(self, clean_databus, custom_dir): executor = MagicMock() From 5a204e3c1272b8d0eab95a90c1ca9fd67e9bf6f5 Mon Sep 17 00:00:00 2001 From: YuanTingHsieh Date: Wed, 8 Jul 2026 18:37:17 -0700 Subject: [PATCH 12/12] Make backend unwind per-step best-effort so close() cannot be skipped _unwind() ran the backend unsubscribes and _client_api.close() in one try block: a failing unsubscribe skipped close(), so the closed gate was never set and an abandoned trainer could still publish results/metrics onto the singleton DataBus (and the dead API stayed subscribed, pinning later jobs' global models). Close the API first -- it is the one step that must not be skipped -- and give every teardown step (close, CLIENT_API_KEY clear, each unsubscribe) its own best-effort block. Tests cover both directions: a failing unsubscribe still closes the API and attempts every remaining unsubscribe, and a failing close() still clears CLIENT_API_KEY and the backend's own subscriptions. --- .../client_api/in_process_backend.py | 51 ++++++++++++------- .../executors/in_process_backend_test.py | 30 +++++++++++ 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/nvflare/app_common/executors/client_api/in_process_backend.py b/nvflare/app_common/executors/client_api/in_process_backend.py index 5fb07d264f..2b02c8fb34 100644 --- a/nvflare/app_common/executors/client_api/in_process_backend.py +++ b/nvflare/app_common/executors/client_api/in_process_backend.py @@ -280,25 +280,38 @@ def finalize(self, fl_ctx: FLContext) -> None: self._unwind() def _unwind(self) -> None: - """Releases DataBus state so nothing leaks into later jobs (DataBus is a process singleton).""" - try: - if self._data_bus is not None: - if self._subscribed: - self._data_bus.unsubscribe(TOPIC_LOCAL_RESULT, self._local_result_callback) - self._data_bus.unsubscribe(TOPIC_LOG_DATA, self._log_result_callback) - self._data_bus.unsubscribe(TOPIC_ABORT, self._to_abort_callback) - self._data_bus.unsubscribe(TOPIC_STOP, self._to_abort_callback) - self._subscribed = False - if self._client_api is not None: - # detach the API's own subscriptions too: the singleton bus would otherwise - # keep the dead instance subscribed to TOPIC_GLOBAL_RESULT, pinning each - # later job's global model via the dead object - self._client_api.close() - if self._data_bus.get_data(CLIENT_API_KEY) is self._client_api: - # only clear our own entry: a later backend may have installed its API - self._data_bus.put_data(CLIENT_API_KEY, None) - except Exception: - self.logger.error(secure_format_traceback()) + """Releases DataBus state so nothing leaks into later jobs (DataBus is a process singleton). + + Each step is individually best-effort: a failure in one must not skip the others. + """ + # close the API FIRST: it detaches the API's own subscriptions (the singleton bus would + # otherwise keep the dead instance subscribed to TOPIC_GLOBAL_RESULT, pinning each later + # job's global model) and sets the closed gate that stops an abandoned trainer from + # publishing into a successor job -- the one step that must not be skipped by a failure + # elsewhere in the teardown + if self._client_api is not None: + try: + self._client_api.close() + except Exception: + self.logger.error(secure_format_traceback()) + try: + if self._data_bus is not None and self._data_bus.get_data(CLIENT_API_KEY) is self._client_api: + # only clear our own entry: a later backend may have installed its API + self._data_bus.put_data(CLIENT_API_KEY, None) + except Exception: + self.logger.error(secure_format_traceback()) + if self._data_bus is not None and self._subscribed: + for topic, callback in ( + (TOPIC_LOCAL_RESULT, self._local_result_callback), + (TOPIC_LOG_DATA, self._log_result_callback), + (TOPIC_ABORT, self._to_abort_callback), + (TOPIC_STOP, self._to_abort_callback), + ): + try: + self._data_bus.unsubscribe(topic, callback) + except Exception: + self.logger.error(secure_format_traceback()) + self._subscribed = False self._client_api = None def _prepare_task_meta(self, fl_ctx: FLContext, task_name: Optional[str]) -> dict: diff --git a/tests/unit_test/app_common/executors/in_process_backend_test.py b/tests/unit_test/app_common/executors/in_process_backend_test.py index 5c625ff47f..4e678ac6e8 100644 --- a/tests/unit_test/app_common/executors/in_process_backend_test.py +++ b/tests/unit_test/app_common/executors/in_process_backend_test.py @@ -258,6 +258,36 @@ def test_unwind_logs_cleanup_failure(self, caplog): assert "unsubscribe failed" in caplog.text + def test_unwind_closes_api_even_when_unsubscribe_fails(self, caplog): + """close() sets the zombie-containment gate (and detaches the API's own subscriptions); + a failing backend unsubscribe must not skip it -- unwind is best-effort per step.""" + backend = InProcessBackend() + backend._data_bus = Mock() + backend._data_bus.unsubscribe.side_effect = RuntimeError("unsubscribe failed") + backend._data_bus.get_data.return_value = None + backend._subscribed = True + api = Mock() + backend._client_api = api + + backend._unwind() + + api.close.assert_called_once() + # every unsubscribe was still attempted, not abandoned at the first failure + assert backend._data_bus.unsubscribe.call_count == 4 + assert "unsubscribe failed" in caplog.text + + def test_finalize_cleans_backend_state_even_when_close_fails(self, clean_databus, custom_dir, caplog): + """The reverse direction: a failing close() must not skip the CLIENT_API_KEY clear or + the backend's own unsubscribes.""" + backend, _ = _initialized_backend(custom_dir) + backend._client_api.close = Mock(side_effect=RuntimeError("close failed")) + + backend.finalize(FLContext()) + + assert "close failed" in caplog.text + assert clean_databus.get_data(CLIENT_API_KEY) is None + assert not _backend_callbacks_subscribed(clean_databus, backend) + class TestExecute: def test_execute_round_trip(self, clean_databus, custom_dir):