Add ClientAPIExecutor with frozen backend spec and working in_process backend#4857
Open
YuanTingHsieh wants to merge 7 commits into
Open
Add ClientAPIExecutor with frozen backend spec and working in_process backend#4857YuanTingHsieh wants to merge 7 commits into
YuanTingHsieh wants to merge 7 commits into
Conversation
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 NVIDIA#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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the new public ClientAPIExecutor entry point and its internal backend contract as an interface-freeze skeleton for the “Client API Execution Modes” program. It establishes a frozen constructor/configuration surface, dispatch scaffolding for in_process / external_process / attach, and a backend context/spec that follow-up PRs will implement.
Changes:
- Add
ClientAPIExecutorskeleton with execution-mode validation, backend creation hooks, clean failure behavior (system_panicon START_RUN), and executor-owned analytics event firing. - Add internal backend contract types:
ClientAPIBackendSpecABC and frozenClientAPIBackendContextsnapshot handed to backends. - Add a comprehensive unit test suite that pins constructor signature/order/defaults and validates mode-scoped argument handling and failure semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
nvflare/app_common/executors/client_api_executor.py |
Adds the public executor skeleton with frozen configuration surface, backend dispatch hooks, and analytics ownership. |
nvflare/app_common/executors/client_api/backend_spec.py |
Introduces the internal backend interface (ClientAPIBackendSpec) and frozen backend context snapshot (ClientAPIBackendContext). |
nvflare/app_common/executors/client_api/__init__.py |
Marks the new client_api package. |
tests/unit_test/app_common/executors/client_api_executor_test.py |
Adds unit tests for validation matrix, dispatch failure behavior, analytics event firing paths, and surface-freeze signature pinning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile SummaryThis PR introduces the
Confidence Score: 5/5Safe to merge. The core DataBus wiring, initialize/finalize lifecycle, monotonic-clock timeout, and DataBus cleanup are all correct and thoroughly tested. The previous round feedback on allow_reconnect normalization, double _backend_registry() call, log-callback exception isolation, and the falsy-guard bypass in _log_result_callback have all been addressed. Two remaining items in in_process_backend.py are non-blocking. nvflare/app_common/executors/client_api/in_process_backend.py Important Files Changed
Reviews (6): Last reviewed commit: "Raise ClientAPIExecutor patch coverage" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4857 +/- ##
==========================================
+ Coverage 56.20% 60.49% +4.29%
==========================================
Files 967 974 +7
Lines 91978 92865 +887
==========================================
+ Hits 51699 56182 +4483
+ Misses 40279 36683 -3596
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
YuanTingHsieh
changed the title
…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). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Interface freeze #2 (EX-2) plus the first working backend (EX-3): the frozen
ClientAPIExecutorconstructor +ClientAPIBackendSpec, and the in_process backend implementing it, with tests that drive the real DataBus round trip.Skeleton (EX-2): one public executor for all Client API execution modes (
in_process/external_process/attach), frozen V1 constructor surface (converter/exchange-format args deliberately excluded per FLARE-2698), per-mode constructor validation, executor-owned analytics hook (fire_log_analytics— the single LOG→analytics ownership point), clean-failure dispatch for not-yet-implemented modes.in_process backend (EX-3): ports
InProcessClientAPIExecutor's DataBus machinery behind the frozen spec — trainer script on a CJ thread, tasks overTOPIC_GLOBAL_RESULT, results overTOPIC_LOCAL_RESULT— with the contract obligations the legacy code lacked:initialize()self-unwinds on failure;finalize()is idempotent and detaches all of the job's DataBus state, including the API instance's own subscriptions via a newInProcessClientAPI.close()(the singleton bus otherwise keeps a dead job's API subscribed, pinning each later job's global model);execute()is bounded byresult_wait_timeouton a monotonic clock; after the in-process trainer aborts (its thread is never relaunched), later tasks fail fast at entry withEXECUTION_EXCEPTIONand the recorded first cause instead of a misleading instantTASK_ABORTED; stale results from a timed-out task are dropped;ExchangeFormat.RAW/TransferType.FULL; DIFF returns with the model_registry transfer-type decision);Tests: 101 for the executor + backend (constructor matrix, surface freeze, dispatch, real-DataBus round trip, multi-round, abort mid-task, post-timeout fail-fast, early/late init-failure unwind, idempotent finalize with exactly one STOP, successor
CLIENT_API_KEYguard, dead-API detach, LOG routing, RAW/FULL meta); 301 green across the executor + client in_process surface; legacy executors untouched.Design:
docs/design/client_api_execution_modes.md(#4853). Plan rows: EX-2 + EX-3.🤖 Generated with Claude Code