From 99c827d542311c8e2a09d9318edefbab5da0b826 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Sat, 16 May 2026 20:32:57 -0400 Subject: [PATCH 1/2] fix(megatron_training_lib): correct training-log parsing and drop sudo from libbnxt cp Three fixes in MegatronLlamaTrainingJob. The first two share a single failure mode (the parser silently sees the wrong slice of training.log and downstream code treats an empty/mid-run dict as PASS); the third is a one-line drive-by in the same file. 1. TRAINING_PROGRESS_PATTERNS matched any `throughput per GPU:` / `tokens/GPU/s` line. Megatron emits those every iteration after the first, so _is_training_complete returned True after iter 1 and the poll loop tail-extracted metrics mid-run. Replaced with a single pattern anchored on `iteration N/ N |`; the `\1` backreference forces current-iter == total-iter, so it fires exactly once per run (at the final iteration line). 2. get_training_results_dict was `cat ... | tail -15` on training.log. Post-training cleanup on a healthy run (aiter import spam, validation eval banner, NCCL teardown warnings) can write well over 15 lines after the final iter, pushing every iteration line out of the tail window. _parse_training_results then matched nothing and returned an empty results_dict, which downstream code treats as PASS. Pre-filter with `grep -E 'iteration N/ N |'` before `tail -15` so the tail window contains the last 15 iteration lines, which is what the parser was always assumed to be fed. 3. exec_nic_setup_scripts broadcom/thor branch runs `docker exec ... /bin/bash -c "sudo cp ... so.host ... so; ..."`. The training container already runs as root, so the nested sudo is unnecessary; on training images that don't install sudo it returns 127 and the libbnxt copy never happens, surfacing later as RDMA/verbs failures. Dropping the sudo prefix makes the command work on both flavors. --- cvs/lib/megatron_training_lib.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/cvs/lib/megatron_training_lib.py b/cvs/lib/megatron_training_lib.py index 03dff963..6a9336dd 100644 --- a/cvs/lib/megatron_training_lib.py +++ b/cvs/lib/megatron_training_lib.py @@ -42,8 +42,7 @@ } TRAINING_PROGRESS_PATTERNS = [ - r'throughput per GPU(?:\s*\([^)]*\))?\s*:|tokens\/GPU\/s\s+[0-9]+', - r'throughput per GPU:|tokens\/GPU\/s\s+[0-9]+', + r'iteration\s+(\d+)/\s+\1\s*\|', ] TRAINING_NAN_PATTERNS = [ @@ -384,7 +383,7 @@ def exec_nic_setup_scripts( # override the gid_index to 3 for broadcom self.nccl_ib_gid_index = 3 out_dict = self.phdl.exec( - f'docker exec {self.container_name} /bin/bash -c "sudo \ + f'docker exec {self.container_name} /bin/bash -c " \ cp /usr/lib/x86_64-linux-gnu/libibverbs/libbnxt_re-rdmav34.so.host \ /usr/lib/x86_64-linux-gnu/libibverbs/libbnxt_re-rdmav34.so; \ sleep 2;ibv_devinfo;sleep 2;"' @@ -581,10 +580,17 @@ def get_training_results_dict( - re is imported in the module scope. """ - # Read the training log output from the "last" node (assumed authoritative) + # Read the training log output from the "last" node (assumed authoritative). + # Filter to megatron iteration lines BEFORE tailing: post-training cleanup + # (aiter import spam, validation eval, NCCL teardown warnings) can otherwise + # push iteration lines out of a `tail -15` window, leaving the parser with + # nothing to match and silently emitting an empty results_dict. last_node = self.host_list[len(self.host_list) - 1] last_node_num = len(self.host_list) - 1 - out_dict = self.phdl.exec(f'cat {self.log_dir}/megatron-logs/out-node{last_node_num}/training.log | tail -15') + out_dict = self.phdl.exec( + f"grep -E 'iteration[[:space:]]+[0-9]+/[[:space:]]+[0-9]+[[:space:]]*\\|' " + f"{self.log_dir}/megatron-logs/out-node{last_node_num}/training.log | tail -15" + ) # Select the log content from the last node output = out_dict[last_node] From f91e6c8eed020052ad879b8ee338f16d117fd408 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Thu, 21 May 2026 14:54:43 -0400 Subject: [PATCH 2/2] Tolerate variable spacing around '/' in iteration patterns Per review on #177: the previous regex/grep both required at least one space after the '/'. Megatron versions that emit 'iteration 100/100 |' (no space) or 'iteration 100 / 100 |' (space on both sides) would have stopped matching, silently regressing _is_training_complete and get_training_results_dict. Switch both to use [[:space:]]*/[[:space:]]* (and the Python equivalent \s*/\s*) so any spacing around '/' is accepted. --- cvs/lib/megatron_training_lib.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cvs/lib/megatron_training_lib.py b/cvs/lib/megatron_training_lib.py index 6a9336dd..005d0bc7 100644 --- a/cvs/lib/megatron_training_lib.py +++ b/cvs/lib/megatron_training_lib.py @@ -42,7 +42,7 @@ } TRAINING_PROGRESS_PATTERNS = [ - r'iteration\s+(\d+)/\s+\1\s*\|', + r'iteration\s+(\d+)\s*/\s*\1\s*\|', ] TRAINING_NAN_PATTERNS = [ @@ -588,7 +588,7 @@ def get_training_results_dict( last_node = self.host_list[len(self.host_list) - 1] last_node_num = len(self.host_list) - 1 out_dict = self.phdl.exec( - f"grep -E 'iteration[[:space:]]+[0-9]+/[[:space:]]+[0-9]+[[:space:]]*\\|' " + f"grep -E 'iteration[[:space:]]+[0-9]+[[:space:]]*/[[:space:]]*[0-9]+[[:space:]]*\\|' " f"{self.log_dir}/megatron-logs/out-node{last_node_num}/training.log | tail -15" )