MEAP review feedback - Ch1 and 2

ACCELERATORS.md

@@ -129,9 +129,10 @@ Key points from the run:
   kernels are not built for `sm_121` (compute capability 12.1). The tested-good
   setup is PyTorch `2.13.0+cu130`.
 - **Recipe fix landed during validation.** The chapter 2 data prep path
-  originally called `reformat_it_answers.py` without required flags. The recipe
-  now passes:
-  `python scripts/reformat_it_answers.py --in data/it_support/train.jsonl --out data/it_support_fmt/train.jsonl`.
+  originally called `reformat_it_answers.py` in a way that failed on older script versions. The recipe
+  now uses:
+  `python scripts/reformat_it_answers.py`.
+  (Since 2026-09-10 the script defaults to processing both train and valid splits; explicit file flags still work.)
 - **Optional OpenRouter warnings are non-fatal.** If `OPENROUTER_API_KEY` is
   unset, the reformat step logs warnings and keeps passthrough rows; the run
   still completes.

DGX-SPARK-READM.MD → DGX-SPARK-README.md

@@ -239,8 +239,8 @@ The new chapter-oriented recipe was exercised on DGX Spark in this environment:
 
 Recipe fix applied during validation:
 
-- Updated data reformat step to pass required args:
-  `python scripts/reformat_it_answers.py --in data/it_support/train.jsonl --out data/it_support_fmt/train.jsonl`
+- Updated data reformat step to process both train and valid splits by default:
+  `python scripts/reformat_it_answers.py`
 
 If you are trying this on a fresh DGX Spark machine, run `setup` first, then
 `smoke`, then chapter commands (`ch1` ... `ch5`) or `all`.

DGX-SPARK-RECIPE.sh

@@ -86,7 +86,7 @@ do_data() {
   ensure_venv
   echo "[data] Building IT-support dataset"
   python scripts/build_it_support_dataset.py
-  python scripts/reformat_it_answers.py --in data/it_support/train.jsonl --out data/it_support_fmt/train.jsonl
+  python scripts/reformat_it_answers.py
 }
 
 do_ch1() {

code/scripts/measure_peak_vram.py

@@ -0,0 +1,72 @@
+"""Measure the peak GPU memory a chapter's training script needs on YOUR hardware.
+
+Runs any training module in-process for a few steps and prints per-device peak
+memory, so you can check a chapter against your card before committing to a run.
+This is how the numbers in ACCELERATORS.md ("GPU requirements at a glance") were
+produced on the book's reference A30s.
+
+Examples (run from code/ with the venv active; pick the GPUs with CUDA_VISIBLE_DEVICES):
+
+    CUDA_VISIBLE_DEVICES=0 python -m scripts.measure_peak_vram chapter05.train_lora \
+        --train data/it_support_fmt/train.jsonl --valid data/it_support_fmt/valid.jsonl \
+        --out /tmp/probe_lora --max_steps 3 --report_to none
+
+    CUDA_VISIBLE_DEVICES=0,1 python -m scripts.measure_peak_vram chapter06.train_sft \
+        --train data/it_support_fmt/train.jsonl --valid data/it_support_fmt/valid.jsonl \
+        --out /tmp/probe_sft --max_steps 3 --report_to none
+
+Everything after the module name is passed to that module unchanged. An
+out-of-memory error is reported as a result, not a crash, with the peak reached
+before the failure. Delete the --out directory afterwards (full SFT writes ~8 GB).
+"""
+from __future__ import annotations
+
+import json
+import runpy
+import sys
+import time
+import traceback
+
+import torch
+
+from common.gpu import peak_gpu_memory
+
+
+def main() -> None:
+    if len(sys.argv) < 2 or sys.argv[1] in ("-h", "--help"):
+        print(__doc__)
+        sys.exit(0)
+    module = sys.argv[1]
+    sys.argv = [module] + sys.argv[2:]
+    t0 = time.time()
+    status, error = "ok", ""
+    try:
+        runpy.run_module(module, run_name="__main__")
+    except SystemExit:
+        pass
+    except BaseException as exc:  # noqa: BLE001 - we want OOM reported as a result
+        status = "ERROR"
+        error = f"{type(exc).__name__}: {str(exc)[:300]}"
+        traceback.print_exc()
+    result = {
+        "module": module,
+        "status": status,
+        "error": error,
+        "seconds": round(time.time() - t0),
+        "torch": torch.__version__,
+        "devices": peak_gpu_memory(),
+    }
+    total = sum(float(d["peak_allocated_gib"]) for d in result["devices"])
+    print("\n=== Peak GPU memory ===")
+    for d in result["devices"]:
+        print(f"  cuda:{d['device']} ({d['name']}): {d['peak_allocated_gib']:.2f} GiB allocated, "
+              f"{d['peak_reserved_gib']:.2f} GiB reserved")
+    if len(result["devices"]) > 1:
+        print(f"  total across GPUs: {total:.2f} GiB (what a single card would need)")
+    if status != "ok":
+        print(f"  run ended with {error}")
+    print("PEAK_VRAM_RESULT " + json.dumps(result))
+
+
+if __name__ == "__main__":
+    main()

code/scripts/reformat_it_answers.py

@@ -20,7 +20,7 @@
 while its token-F1 is still scored against the original human answers.
 
     python scripts/reformat_it_answers.py                 # both splits
-    python scripts/reformat_it_answers.py --in X --out Y  # one file
+    python scripts/reformat_it_answers.py --input X --output Y  # one file
 
 Needs OPENROUTER_API_KEY (see code/README.md). Both output files are committed to
 the repo, so you only need to run this if you rebuild the dataset from source.
@@ -142,16 +142,16 @@ def qa(row):
 
 def main():
     ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter)
-    ap.add_argument("--in", dest="inp", default=None,
-                    help="input split from build_it_support_dataset.py; with no --in/--out both the train and "
+    ap.add_argument("--in", "--input", dest="inp", default=None,
+                    help="input split from build_it_support_dataset.py; with no --input/--output both the train and "
                          "valid splits are processed (the default the READMEs rely on)")
-    ap.add_argument("--out", default=None, help="output path (required if --in is given)")
+    ap.add_argument("--out", "--output", dest="out", default=None, help="output path (required if --input is given)")
     ap.add_argument("--limit", type=int, default=0, help="0 = all")
     ap.add_argument("--dry", action="store_true", help="print, do not write")
     ap.add_argument("--workers", type=int, default=8, help="concurrent API calls")
     args = ap.parse_args()
     if (args.inp is None) != (args.out is None):
-        ap.error("--in and --out must be given together (or neither, to process both default splits)")
+        ap.error("--input and --output must be given together (or neither, to process both default splits)")
     pairs = [(args.inp, args.out)] if args.inp else SPLITS
     for inp, out in pairs:
         process(inp, out, args.limit, args.dry, args.workers)

code/tests/test_import_smoke.py

@@ -0,0 +1,52 @@
+"""Import-smoke test.
+
+Imports every module that pulls the heavy ML dependency chain (peft, trl,
+transformers) so CI catches dependency-resolution breaks on a fresh install --
+the kind the chapter test suites miss because they only import lightweight data
+and metrics modules.
+
+This exists because a fresh install once resolved transformers to 5.x, which
+removed ``HybridCache``; ``peft<0.18`` imports that at load time, so
+``import peft`` (and the chapter 2 quickstart) failed everywhere -- yet CI
+stayed green because no test imported peft. These imports are GPU-free and do
+no training; they only exercise the import graph.
+
+Deliberately excluded:
+  - chapter05.train_qlora (imports bitsandbytes, a CUDA-only extra)
+  - chapter03.ch03_data_quality_explore (a script with no __main__ guard, so
+    importing it would run the full experiment)
+"""
+from __future__ import annotations
+
+import importlib
+import sys
+from pathlib import Path
+
+import pytest
+
+# Make the code/ root importable when running pytest without an editable install.
+_code_root = Path(__file__).resolve().parent.parent
+if str(_code_root) not in sys.path:
+    sys.path.insert(0, str(_code_root))
+
+MODULES = [
+    "chapter02.quickstart",
+    "chapter05.modeling",
+    "chapter05.train_lora",
+    "chapter06.train_sft",
+    "chapter07.train_student",
+    "chapter08.train_dpo",
+    "chapter09.safety_monitor",
+]
+
+
+@pytest.mark.parametrize("module", MODULES)
+def test_module_imports(module: str) -> None:
+    """Importing the module must not raise (catches dependency-resolution breaks).
+
+    Chapters not present in this checkout are skipped, not failed: the public repo
+    publishes chapters as the MEAP releases them, so a chapter's package may be absent.
+    """
+    if not (_code_root / module.split(".")[0]).is_dir():
+        pytest.skip(f"{module.split('.')[0]} is not in this checkout")
+    importlib.import_module(module)