Added primus_lib.py - #277
Conversation
solaiys
left a comment
There was a problem hiding this comment.
Reviewed for blocking issues + whether it follows the CVS node command-execution convention.
🔴 Blocking — factory.py has a broken import; the dispatcher is un-importable (see inline comment on factory.py). from cvs.lib.training.megatron.megatron_lib import MegatronTrainingJob points at a module that doesn't exist — the real Megatron lib is the flat cvs/lib/megatron_training_lib.py and its class is MegatronLlamaTrainingJob (not MegatronTrainingJob). So importing create_training_job raises ModuleNotFoundError.
Node command-execution convention (the consistency check requested)
primus_lib.pyitself follows the modern CVSorch-based convention used by the current inference suites (vllm/sglang):orch.exec(...)/orch.exec_cmd_list([...])for in-container execution across nodes, andorch.all.exec(...)/orch.all.exec_cmd_list([...])for bare-host steps (writing volume-mounted wrapper scripts,date, RDMA/ethtool stat collection), with per-node indexing viarange(len(orch.hosts)). The host-writes-script / container-launches-script split is coherent and matches howvllm_job.pydrives nodes. ✅- The inconsistency is in
factory.py, not the exec model: it dispatches to a Megatron module/class and an(orch, variant_config, **kwargs)constructor that the existingMegatronLlamaTrainingJob(phdl/docker_lib-based) does not provide — i.e. the factory is written against a future/refactored Megatron that isn't in the tree. Please reconcile both the import path/class name and the constructor contract.
Non-blocking notes
- Dead NaN/Inf check in
verify_training_results(primus_lib.py~L663–670):for result_list in self.training_results_dict[result_key]: for result_val in result_list:— the dict values arelist[str], so the inner loop iterates over characters andre.search('nan|inf', <single char>)never matches. Should be a single loop (for result_val in self.training_results_dict[result_key]:). It's redundant with the working poll-time_has_nan_inf_results(), so no false PASS in the common path — but a NaN in an uncovered metric (e.g.grad_norm) would slip through both. home_dir = os.path.expanduser("~")is expanded on the runner and then used verbatim in remote host/container paths (log_dir/scripts_dir/data_cache_dirdefaults). Fine when homes are homogeneous/NFS-mounted, fragile otherwise.- Minor:
start_training_job(timeout=500)— thetimeoutparameter is never used.
| """Instantiate the correct TrainingJob class based on image and framework. | ||
|
|
||
| Dispatch logic: | ||
| 1. If container image contains "primus" → PrimusTrainingJob, with the |
There was a problem hiding this comment.
🔴 Blocking — this import fails at module load; factory.py (and thus create_training_job) is un-importable. cvs/lib/training/megatron/megatron_lib.py does not exist — there is no cvs/lib/training/megatron/ package. The existing Megatron lib is the flat cvs/lib/megatron_training_lib.py, and its class is named MegatronLlamaTrainingJob, not MegatronTrainingJob. Existing training suites import it as from cvs.lib import megatron_training_lib. As written this raises ModuleNotFoundError (and, even with the path fixed, ImportError on the class name). Fix: from cvs.lib.megatron_training_lib import MegatronLlamaTrainingJob (or add the restructured cvs/lib/training/megatron/megatron_lib.py in this PR) — and reconcile the constructor, since create_training_job calls MegatronTrainingJob(orch, variant_config, **kwargs) but MegatronLlamaTrainingJob uses the older phdl/docker_lib model.
0195210 to
dd76cd2
Compare
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
Signed-off-by: amd-droy <droy@amd.com>
* Add IX Run Deck reporting stack on top of dev/dtni. Rebased onto dev/dtni after IX-atom merge (#238). Keeps only Run Deck wiring and add-only doc updates; no unrelated IX-atom or utils doc churn. * Expand sweep charts with per ISL/OSL grouping and add vLLM preset. Group concurrency charts by sequence shape so multi-shape sweeps stay readable, add tail-latency chart presets, and wire vllm_single auto-registration. * fix(report): register inference suite preset before session bind Re-sync suite stem from pytest args at session start so auto-discovery works when tests run from the installed package. Log when no preset is registered instead of failing silently, and wire inferencex_atom explicitly in suite conftest as a fallback. * fix(report): link run-card pytest HTML and log via sibling basenames. * feat(report): add chart_comparison payload for multi-shape sweeps. Expose cross-shape series on a shared concurrency axis so the interactive viewer can render grouped comparisons without duplicating chart_series logic. * feat(report): add cross-shape sweep chart render helpers. Add grouped-bar and line comparison renderers plus shared tooltip CSS for static HTML chart surfaces. * feat(report): improve static run deck bar charts with axis grid. Add y/x gridlines, aligned tick labels, hover tooltips, and a viewer banner so cross-shape comparison stays in the interactive sidecar. * feat(report): expand interactive viewer with cross-shape comparison. Add grouped bar and line comparison modes, filter-aware heatmap and gates, and overview layout updates backed by chart_comparison payload tests. * feat(report): expose sweep_ttft_metric in viewer payload. Lets the interactive viewer default the throughput-vs-latency Y-axis to the suite TTFT metric. * feat(report): add percentile fans, gate margins, and heatmap toggle to viewer. Add P90/P95/P99 fan charts and gate margin vs concurrency, move throughput-vs-latency to the bottom, and let the heatmap switch between throughput and latency metrics. * refactor(report): drop unused static cross-shape chart renderers. Keep sweep_charts as tooltip CSS only; cross-shape comparison lives in the Chart.js viewer. * refactor(report): dedupe viewer charts and simplify multi-shape banner. Use sweep_has_multi_shape_comparison for the static deck banner, limit per-shape viewer lines to throughput and means, and document viewer chart surfaces in README. * Restore CI one-page summary HTML for inference suite reports. Bring back {report_basename}_summary.html generation and pytest bundle links, and extend the IX atom sample generator with a multi-shape variant. * Restore viewer baseline comparison and consolidate report docs. Bring back prev-run delta columns and payload wiring, remove ADDING_A_SUITE.md, and point suite/report docs at cvs/lib/report/README.md. * refactor(report): consolidate shared sweep shape and comparison helpers. Deduplicate shape grouping, prev-run comparison, preset builders, and viewer chart utilities without changing report behavior. * docs(inference): restore ADDING_A_SUITE.md with Run Deck step. Keep the existing suite guide on dev/dtni and retain the optional Step 8 report wiring section instead of deleting the file. * refactor(report): trim PR scope by deferring non-IX-atom extras. Remove the local sample generator and vLLM preset, shorten README, and drop unused prev-run HTML renderer and panel_shell helper. * refactor(report): rely on auto-register for IX-atom preset wiring. Drop explicit pytest_configure registration from the inferencex_atom suite conftest and expose a single stem-matched preset export. * refactor(report): dedupe cell-card CSS and drop chart_comparison pre-bake. Unify pytest and static cell-card themes, build cross-shape comparison only in the viewer, and trim overlapping unit tests. * fix(report): use bundle-relative links and omit config run-card notes. Write run deck artifacts into the pytest bundle folder, link pytest HTML and run logs correctly after zip extract, and stop auto-rendering variant run_card.notes in static HTML. * fix(report): address PR #244 review feedback. Merge duplicate pytest_configure hooks, escape HTML in report logs, and tighten report payload binding and tests. * fix(conftest): rename report bind fixture to match module scope. Addresses PR #244 fixture-scope review: name no longer implies session-scoped pytest fixture. * fix(report): align PR #244 inline review fixes. Use tryfirst pytest_configure, session-scoped report store with module bind merge, and key-is-None results column offset. * fix(report): sanitize absolute paths in link_or_text_html. Only treat ../ paths as bundle-relative so local filesystem paths reduce to basename in HTML reports.
* Integrating nodesmoke tier 1 tests using Primus cli * Fixed the braces * Updated the README * Added the robustness improvement Signed-off-by: Urvashi Tiwari <urtiwari.com>
Signed-off-by: sukesh kalla <skalla@amd.com>
Signed-off-by: sukesh kalla <skalla@amd.com>
41f2788 to
bc3954a
Compare
…n_single
- Fix tokens_per_gpu pattern to match actual log format
('tokens/s/GPU inst/harmonic mean' instead of 'tokens per GPU (tokens/s/GPU)')
- Export NCCL_SOCKET_IFNAME and GLOO_SOCKET_IFNAME for single-node runs
- Export PRIMUS_TURBO_ATTN_V3_ATOMIC_FP32 and NVTE_CK_IS_V3_ATOMIC_FP32
for MI300X and MI325X (single and distributed)
- Replace MegatronTrainingJob with create_training_job in megatron_single.py
- Skip tokenizer download when container image is Primus
Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Motivation
Technical Details
Test Plan
Test Result
Submission Checklist