Add HuggingFace Trainer Client API - #4948
Conversation
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge; all previously identified DDP hang classes are addressed and the new code is well-structured with thorough test coverage. The DDP error-broadcast issues flagged in earlier review rounds have been properly fixed: is_running(), _ensure_task(), _capture_budget_if_needed(), and the checkpoint-injection/state-persistence paths all now broadcast rank-0 errors to all ranks before anyone proceeds. The remaining comments are quality suggestions that do not affect correctness in the expected NVFlare deployment environment. Files Needing Attention: nvflare/app_opt/hf/api.py deserves a second look for the _train_with_evaluation_enabled() and _params_exchange_strategy() default choices; all other files are straightforward. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User Script
participant Rank0 as Rank 0 (HFTaskState)
participant RankN as Rank N (HFTaskState)
participant HFTrainer as HF Trainer
participant FlareAPI as NVFlare Client API
User->>Rank0: flare.patch(trainer)
User->>RankN: flare.patch(trainer)
note over Rank0,RankN: _init_client_api_for_rank, register FLCallback
loop FL Rounds
User->>Rank0: flare.is_running()
Rank0->>FlareAPI: flare_api.is_running()
FlareAPI-->>Rank0: True/False
Rank0->>RankN: "broadcast_object(status, src=0)"
User->>Rank0: trainer.evaluate() wrapped_evaluate
Rank0->>FlareAPI: flare_api.receive()
FlareAPI-->>Rank0: "FLModel (task=train)"
Rank0->>RankN: _broadcast_task_payload(payload)
Rank0->>Rank0: _load_global_params_once()
RankN->>RankN: _load_global_params_once()
Rank0->>HFTrainer: original_evaluate()
RankN->>HFTrainer: original_evaluate()
User->>Rank0: trainer.train() wrapped_train
note over Rank0,RankN: _capture_budget_if_needed (rank0 broadcasts)
note over Rank0,RankN: checkpoint injection OR in-memory weight override
Rank0->>HFTrainer: original_train()
RankN->>HFTrainer: original_train()
HFTrainer->>Rank0: FLCallback.on_train_end()
Rank0->>Rank0: _run_rank_zero_operation(send model)
Rank0->>FlareAPI: flare_api.send(FLModel)
Rank0->>RankN: "broadcast_object(ok_status, src=0)"
end
User->>Rank0: flare.is_running() False
note over Rank0,RankN: Loop exits
Reviews (25): Last reviewed commit: "Align HF round metric filtering" | Re-trigger Greptile |
a67974c to
12d1534
Compare
|
/build |
3 similar comments
|
/build |
|
/build |
|
/build |
bb4b5af to
b6d0ea3
Compare
|
/build |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4948 +/- ##
==========================================
+ Coverage 63.30% 63.59% +0.29%
==========================================
Files 1021 1026 +5
Lines 100386 101795 +1409
==========================================
+ Hits 63549 64741 +1192
- Misses 36837 37054 +217
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:
|
|
/build |
1 similar comment
|
/build |
a5c1e5f to
e977894
Compare
|
/build |
|
/build |
2792c1e to
fb926cf
Compare
|
/build |
|
The persistent checkpoint provenance appears to support exactly one additional lifecycle: With the default This also does not currently provide crash recovery. If the Trainer subprocess crashes, Could we narrow Phase 1 to the existing Lightning contract?
This retains the useful HF checkpoint mechanism without introducing an untested recovery contract. |
|
Two additional findings: Blocking: stateless rounds report zero training steps after round 0At With This silently causes:
For stateless mode, Documentation: weight-override escape hatch is undiscoverable
Please either document |
|
Suggestion on example placement: let's move Rationale:
Mechanical changes this implies:
|
pcnudde
left a comment
There was a problem hiding this comment.
Behavioral findings from pinned-head QA using real Transformers/TRL trainers, real two-process Gloo, and two-site/two-round federated runs.
|
/build |
|
Addressed the P1 DDP receive-path hang in b3bcd92: rank 0 now wraps the full task receive/classification/server-prefix strip path, and for distributed runs broadcasts a task-dispatch failure payload when receive(), _read_task_kind(), or strip_server_key_prefix() fails before normal task broadcast. Nonzero ranks now receive the rank-0 error and raise instead of blocking in _broadcast_object. Single-process behavior is preserved: non-AgentClosed receive protocol errors still raise the original exception. Added fake-distributed rank-0/rank-1 regressions plus a real two-process Gloo rank-0 receive-failure regression. Validation: python -m pytest tests/unit_test/app_opt/hf/task_dispatch_contract_test.py -q; python -m pytest tests/unit_test/app_opt/hf/distributed_contract_test.py -q; python -m pytest tests/unit_test/app_opt/hf -q; ./runtest.sh -s. |
|
/build |
|
Addressed the P1 DDP is_running hang in 908abd7: distributed is_running() now catches unexpected rank-0 Client API errors, logs the failure, broadcasts a falsey value, and returns False on all ranks instead of leaving nonzero ranks blocked in broadcast_object_list. Single-process behavior is preserved by re-raising the original exception when world_size <= 1. Added fake-distributed rank-0/rank-1 regressions plus a real two-process Gloo rank-0 is_running-failure regression. Validation: python -m pytest tests/unit_test/app_opt/hf/task_dispatch_contract_test.py -q; python -m pytest tests/unit_test/app_opt/hf/distributed_contract_test.py -q; python -m pytest tests/unit_test/app_opt/hf -q; ./runtest.sh -s. |
|
/build |
|
Addressed the latest review findings in
Validation:
|
|
/build |
|
/build |
holgerroth
left a comment
There was a problem hiding this comment.
Nice support for HF!
|
I took another pass focused on what is actually blocking versus what is architectural simplification. Blocking correctness issue
Please either:
A real regression should construct Trainer with a distinctive optimizer such as SGD plus a custom scheduler and verify that the configured behavior is not silently replaced. HF documents prebuilt optimizer/scheduler support here: https://huggingface.co/docs/transformers/main/optimizers Non-blocking simplification concerns
So my merge position is: fix the prebuilt optimizer/scheduler behavior; the transport/module cleanup can be follow-up work if we intentionally accept the present complexity and shared-storage contract. |
|
Addressed this review in
I retained file staging as an explicit optimization rather than replacing it with direct tensor collectives in this PR. A tensor-collective implementation changes dtype/device/key metadata synchronization and should be evaluated separately against large-model workloads. Validation:
|
|
/build |
|
Addressed the generated training-argument quoting finding in |
|
/build |
|
Addressed the two remaining quality observations in
Validation: |
|
/build |
Summary
nvflare.client.hf.patch()for HuggingFaceTrainer/ TRLSFTTrainerfederated rounds.examples/advanced/hf_client_apiQwen SFT/PEFT example using the defaultflare.patch(trainer)API and a standardprepare_data.pydata-prep step, while leaving the legacyexamples/advanced/llm_hfexample unchanged.3rdParty/packaging.LICENSE.txtfile for the optionalpackagingdependency.Testing
python -m pytest tests/unit_test/app_opt/hf -qpython -m pytest tests/unit_test/recipe/fedavg_recipe_test.py -qpython -m py_compile examples/advanced/hf_client_api/client.py examples/advanced/hf_client_api/model.py examples/advanced/hf_client_api/prepare_data.py examples/advanced/hf_client_api/job.pypython examples/advanced/hf_client_api/client.py --helppython examples/advanced/hf_client_api/prepare_data.py --helppython examples/advanced/hf_client_api/prepare_data.py --n_clients 2 --data_root /private/tmp/nvflare_hf_client_api_qwen_data_preppython examples/advanced/hf_client_api/job.py --helppython examples/advanced/hf_client_api/job.py --export_config --job_dir /private/tmp/nvflare_hf_client_api_qwen_job_prepared --data_root /private/tmp/nvflare_hf_client_api_qwen_data_preppython3 -u custom/client.py ...rather than an absolute source path undercustom/git diff --check upstream/main...HEAD./runtest.sh -s./runtest.sh -lReview Notes
negate_key_metricpass-through was split into separate PR Expose negate_key_metric in FedAvg recipes #4953. This HF PR keeps the Qwen example independent by disabling recipe best-model selection withkey_metric="".SFTTrainerinstances and appliesflare.patch(), but does not run a real TRL train/send round. The train/send path is covered by realtransformers.Trainerand fake-contract tests.train_with_evaluationhard-fail policy currently depends on existing Client API config. The new HF example explicitly callstrainer.evaluate()beforetrainer.train()each round; recipe-level pass-through can be added later if we want every recipe user to get a hard failure when pre-train eval is omitted.