Fast-LLM integration: test suite - #156
Open
jlamypoirier wants to merge 5 commits into
Open
Conversation
Apply the coarse + fine review findings: - Delete test_actor_error_handling.py — every test reimplemented the actor error-handling logic inline and asserted on the copy, so it could not catch a regression in the real actor. - Drop the generic-path topology matrix (TestWeightUpdateTP2, TestWeightUpdateMultiActor); equivalent multi-topology coverage lives on the Fast-LLM path in test_vllm1_fast_llm_broadcast.py. - Move the EngineManager import out of conftest top level into the factory fixture so the CPU-only unit tests collect without vLLM/torch. - Consolidate the duplicated stream_process_output / kill_process_tree helpers into server_weight_update_utils.py and drop the now-pointless injected stream_process_output_fn parameter. - Remove dead helpers (force_kill_process, check_pattern_detected), unused fixtures/params/imports, and the dead timeout parameter. - Minor typing and style fixes (builtin generics, strict zip, f-string and implicit-Optional cleanups). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r exit code - Delete TestPodIPExchange, TestHostfileCreation, TestRedisHostMultiNode: each asserted on a hand-copy of launch.py logic (_simulate_pod_ip_exchange, _compute_streams_host, the hostfile host-list) rather than the real code path, so a regression in launch.py would leave them green. The helper _simulate_pod_ip_exchange stays as setup for TestDeepSpeedCommand, which drives the real _run_finetune_deepspeed. - Assert trainer_proc.returncode in (0, None) after the wait loop in the server / broadcast pattern tests, so a trainer that crashes during final cleanup fails instead of passing on the already-captured pattern. - Fine cleanups: drop unused `import tempfile`, redundant local re-imports (argparse-as-ap, omegaconf, per-function pathlib/sys.path), unused thread handles and loop counter; hoist the broadcast import out of the inner closure; return the dtype string directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…suite run Running the suite for the first time exposed 11 failures in test_world_multinode.py, all test-setup gaps (not product bugs): - 8× `Missing key wandb_name`: the fast-llm test cfgs (_capture_cmd, _capture_fast_llm_files) omitted wandb.wandb_name, which _run_finetune_fast_llm reads. Add it (None) so the code reaches the torchrun/naming assertions. - 3× `WorldMap has no dns_address_map`: multinode finetune always runs after _exchange_pod_ips (which sets dns_address_map), but _make_world_map produced a pre-exchange map. Set dns_address_map for world_size > 1 in the helper to mirror production. Delete test_deepspeed_multinode_uses_dns_names_without_exchange: its "without exchange" premise is unreachable for multinode, and the after-exchange sibling already covers the DNS-in-filter assertion (plus the no-pod-IP-leak check). Verified on GPU: test_world_multinode + test_launch_process_monitoring now 29 passed. Co-Authored-By: Claude Opus 4.8 <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
1 participant
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.
Claude Opus 4.8 authored this PR description.
Test layer of the Fast-LLM integration stack, extracted from #140:
main ← #151 ← #153 (core integration) ← #155 (configs & metrics) ← this PRWhat this adds
The integration test suite for the Fast-LLM trainer path, layered on top of the code it exercises (#153 core, #155 configs/metrics):
test_vllm1_integration.py,test_vllm1_fast_llm_broadcast.pyand their helpers (vllm_engine_helper.py,server_weight_update_utils.py,weight_update_utils.py) cover the persistent-NCCL weight-update path.test_world_multinode.py,distributed_trainer_helper.py,sync_helper.py.test_actor_error_handling.py,test_launch_process_monitoring.py.test_model_version.py.conftest.py,trainer_test_utils.py,fast_llm_trainer_helper.py.Notes
main.🤖 Generated with Claude Code