[None][chore] Update AD onboard skill and reviewer agent (#241)

* [None][chore] Update AD onboard skill and reviewer agent

- SKILL.md: Add GPU memory sanity check (Phase 0 Step 0) before onboarding
- SKILL.md: Clarify HF reference strategy — import from HF cache via importlib
  instead of copying class definitions into test files
- SKILL.md: Tighten Phase 9/10/11 to consolidate --use-registry guidance and
  remove duplicate mandatory-warning blocks; simplify summary report format
- SKILL.md: Phase 4 note that AD factory already calls AutoConfig.from_pretrained
- ad-onboard-reviewer.md: Add BB section for vision/multi-modal support checks
- ad-onboard-reviewer.md: Clarify B2 custom config justification criteria
- ad-onboard-reviewer.md: F4 — no standalone HF-like class definitions in tests;
  must import from actual HF source

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Address PR review feedback on skill/agent updates

- SKILL.md line 110: Relax "always use HF classes" to "use if they exist"
- SKILL.md line 112: Restore fallback to copying from HF source when HF cache
  unavailable; keep strong preference for HF cache import first
- SKILL.md Phase 9: Restore ⚠️ MANDATORY --use-registry block
- SKILL.md Phase 9 failure steps: Restore bold registry config yaml, items 4+5
- SKILL.md Phase 10: Restore ⚠️ MANDATORY block and item 9 (raw prompts)
- SKILL.md Phase 11: Restore MUST list with raw prompts requirement
- SKILL.md Key Gotchas: Restore bold MUST/NEVER RoPE cos/sin guidance
- reviewer F4: Relax to flag standalone class defs only when viable HF alt exists

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Address second round of PR review feedback

- reviewer BB3 → BB2 (nit rename)
- SKILL.md line 205: remove "first sentence" about --use-registry flag
- SKILL.md line 112: drop overly specific importlib/sys.path sentence

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Require pytest results to be included in PR

Phase 11: agent must run the pytest command on the latest commit
and include results in the PR, not just provide the command.

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Add E2E sanity check note for custom config class in Phase 4

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Reviewer B2: replace Eagle example with E2E heuristic for custom config

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Reviewer B2: remove E2E heuristic, keep AutoConfig.from_pretrained check

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Revert HF cache approach; restore copy-into-test-file for missing HF modules

HF cache paths are not dependable in CI. When HF modules are not in
the installed transformers, copy minimal faithful class definitions from
the HF source into the test file instead of loading from HF cache.
Update reviewer F4 accordingly.

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Extend HF reference strategy to cover trust_remote_code config classes

Clarify that the "copy into test file" pattern applies to config classes
too: when a model's config uses trust_remote_code (not in transformers),
copy a minimal faithful version into the test file rather than loading
from the HF cache. The modeling file itself should not import the config.

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

* [None][chore] Address PR review comments on skill config/test guidance

- Remove overly specific examples from config import note (line 96)
- Simplify trust_remote_code config guidance to avoid multi-modal specifics (line 112)

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

---------

Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>

Author: govind-ramnarayan

.claude/agents/ad-onboard-reviewer.md

@@ -28,13 +28,22 @@ Read the actual source code for each check. Cite `file:line_number` for every PA
 | # | Check | How to verify |
 |---|-------|---------------|
 | B1 | No imports from other AD custom models (`from .modeling_*`) | Grep for `from .modeling_` — only `from .` imports of non-model utilities are OK (e.g., `mla_rope_utils`) |
-| B2 | Config class is imported from `transformers` whenever possible — NOT recreated/copied into the modeling file when it already exists in transformers or is bundled with the checkpoint | Check where the config class comes from. If a `from transformers import ...Config` would work, the file should use it. A locally-defined config class when one is available in `transformers` is a FAIL. |
+| B2 | Config class is imported from `transformers` whenever possible — NOT recreated/copied into the modeling file when it already exists in transformers or is bundled with the checkpoint | Check where the config class comes from. If a `from transformers import ...Config` would work, the file should use it. A locally-defined config class when one is available in `transformers` is a FAIL. A custom config class is only justified when: (a) the config does not exist in `transformers` and cannot be loaded via `AutoConfig.from_pretrained`, or (b) the model code requires config attributes not present in the checkpoint's `config.json` and not available in the standard HF class. If neither applies, the custom class is a FAIL. |
 | B3 | If config is truly unavailable (not in `transformers`, not in checkpoint), file defines a minimal config class and calls `AutoConfig.register()` | Grep for `AutoConfig.register` — should only exist when the config is genuinely missing from transformers |
 
 ### BA Checkpoint compatibility
 | BA1 | Make sure the custom modeling code nn.module hierarchy matches the model hierarchy that is expected in the checkpoint safetensor json. |
 | BA2 | If our modeling code has expert-list style moe experts and the checkpoint has fused moe experts, add a load hook to load the safetensors correctly into our expert list weights.
 
+### BB. Vision / Multi-Modal Support
+
+| # | Check | How to verify |
+|---|-------|---------------|
+| BB1 | If the model has a vision tower (multi-modal), the full `nn.Module` hierarchy for the vision component is present in the modeling file — it is NOT omitted, stubbed out, or replaced with a `pass` body | Grep for vision-related class names (e.g., `VisionTower`, `ViT`, `CLIPVision`, `SiglipVision`) from the HF source. If the model is multi-modal and none appear, flag as FAIL. |
+| BB2 | The test file asserts that vision-related weight keys are present in the model's `state_dict` after `load_state_dict` | Grep the test file for assertions on vision weight key names (or a check that vision-prefixed keys are in the loaded state_dict). Absence of any such assertion is a FAIL for multi-modal models. |
+
+Note: BB1–BB2 only apply if the HF source indicates the model is multi-modal (has image/audio inputs). Mark N/A with justification for pure language models.
+
 ### C. Ops & Compatibility (STRICT — canonical ops are the backbone of AD)
 
 | # | Check | How to verify |
@@ -76,7 +85,7 @@ Mark as N/A with justification if the model doesn't have the relevant component.
 | F1 | Uses small config (hidden_size ~64, num_hidden_layers 2-3, vocab_size ~1000) | Read the test config creation |
 | F2 | No smoke tests — every test has meaningful assertions (`assert_close`, `assert_rmse_close`, shape checks, finiteness checks) | Check each test for substantive assertions |
 | F3 | Do not rely on only `isnan`/`isinf` checks; include functional equivalence assertions | Check tests use `assert_close` or `assert_rmse_close` against reference outputs |
-| F4 | Test imports must be self-contained (transformers imports or copied reference classes only); no hardcoded local/temp path imports | Inspect imports and helper loaders |
+| F4 | Test imports must be self-contained (transformers imports or copied reference classes only); no hardcoded local/temp path imports. If HF modules exist in the installed `transformers`, they must be imported from there. If not, minimal class definitions must be copied faithfully from the HF source into the test file. No standalone class definitions that mirror the model architecture when the class is available in `transformers` — such definitions are effectively a second AD IR and cannot catch bugs shared between both implementations. If any class is defined in the test file that replicates model architecture AND the class is importable from `transformers`, flag as FAIL. | Inspect all class definitions and imports in the test file. |
 
 ### G. Test File — Hierarchical Levels
 

.claude/skills/ad-model-onboard/SKILL.md

@@ -10,6 +10,14 @@ description: Translates a HuggingFace model into a prefill-only AutoDeploy custo
 ## Phase 0 — Gather All Resources Upfront
 Web/GitHub fetches require user approval and the user may leave. Do ALL network access now and save locally before proceeding.
 
+### Step 0 — GPU memory sanity check
+
+Before anything else, check whether the model can fit on the current system.
+
+1. Run `nvidia-smi --query-gpu=memory.total --format=csv,noheader,nounits` to get the total VRAM (in MiB) across all GPUs on the system.
+2. Estimate the model's memory footprint from the HuggingFace model card or config (number of parameters × bytes per parameter, e.g. 7B params × 2 bytes = ~14 GB for bfloat16).
+3. If the estimated size exceeds total system VRAM, **stop and report this to the user** — do not proceed with onboarding until the user acknowledges and decides how to proceed. Example message: "This model requires ~Xgb but the system only has Ygb across N GPUs. Onboarding is likely to fail at the e2e run stage."
+
 **Step 1 — Check local transformers install first:**
 ```bash
 python -c "import transformers; print(transformers.__file__)"
@@ -85,8 +93,8 @@ Available canonical ops (see `tensorrt_llm/_torch/auto_deploy/custom_ops/README.
 ## Phase 4 — Register
 1. Bottom of model file: `AutoModelForCausalLMFactory.register_custom_model_cls("ConfigClassName", ForCausalLM)`.
 2. Add import + `__all__` entry in `models/custom/__init__.py`.
-3. **Prefer reusing the existing config class** — if the config can be loaded via `AutoConfig.from_pretrained(model_id)` (either from the installed `transformers` or from files in the HF cache downloaded in Phase 0), import it from `transformers` and use it directly. Do NOT recreate or copy the config class into the modeling file when it is already available.
-4. Only if the config is truly not available (not in `transformers` and not bundled with the checkpoint), define a minimal config class in the modeling file and `AutoConfig.register(model_type, ConfigCls, exist_ok=True)`.
+3. **Prefer reusing the existing config class** — if the config can be loaded via `AutoConfig.from_pretrained(model_id)` (either from the installed `transformers` or from files in the HF cache downloaded in Phase 0), import it from `transformers` and use it directly. Do NOT recreate or copy the config class into the modeling file when it is already available. Note: AD's factory already calls `AutoConfig.from_pretrained(model_id, trust_remote_code=True)` and passes the result to your model, so you rarely need to import the config at all — if you find yourself doing so, sanity-check that it's genuinely needed.
+4. Only if the config is truly not available (not in `transformers` and not bundled with the checkpoint), define a minimal config class in the modeling file and `AutoConfig.register(model_type, ConfigCls, exist_ok=True)`. A good sanity check: if the E2E test passes without a custom config class, you don't need one — `AutoConfig.from_pretrained` already picked up the right class.
 
 ## Phase 5 — Model Input Contract
 The custom model's forward signature must follow these rules:
@@ -99,9 +107,9 @@ The custom model's forward signature must follow these rules:
 ## Phase 6 — Hierarchical Tests
 Create `tests/unittest/_torch/auto_deploy/unit/singlegpu/models/test_{name}_modeling.py`. Use `test_glm4_moe_lite_modeling.py` as template. **No smoke tests.** Small config (hidden=64, layers=2-3, vocab=1000). Use `pytest.skip` if HF class unavailable.
 
-**HF Reference Strategy:** Equivalence tests compare our custom implementation against the HF reference with identical weights and inputs.
+**HF Reference Strategy:** Equivalence tests compare our custom implementation against the HF reference with identical weights and inputs. **Use actual HF classes if they exist — prefer importing directly over standalone HF-like implementations for unit tests.** Standalone "reference" implementations are effectively alternative AD IR models and defeat the purpose of the reference test; they also tend to silently agree with whatever bugs exist in the custom model.
 - **If HF modules exist in the installed `transformers`**: import them directly (e.g., `from transformers.models.deepseek_v3.modeling_deepseek_v3 import DeepseekV3ForCausalLM`). Wrap imports in `_get_hf_*_class()` try/except helpers that return `None` on `ImportError`, and use `pytest.skip` when `None`.
-- **If HF modules are NOT in the installed `transformers`**: copy the minimal module definitions from the HF `modeling_*.py` source into the test file as standalone reference classes. This keeps tests self-contained without requiring a specific `transformers` version. **Important**: make sure the copy is minimal and strictly faithful to the HF implementation only. Do NOT tweak the functionality of the reference.
+- **If HF modules are NOT in the installed `transformers`**: copy the minimal module definitions from the HF `modeling_*.py` source into the test file as standalone reference classes. This keeps tests self-contained without requiring a specific `transformers` version or HF cache at test time. **Important**: make sure the copy is minimal and strictly faithful to the HF implementation only. Do NOT tweak the functionality of the reference. The same applies to **config classes** that use `trust_remote_code` (i.e., not available in `transformers`): copy a minimal faithful version into the test file. The modeling file should NOT import the config class — AD loads it at runtime via `AutoConfig.from_pretrained(..., trust_remote_code=True)`. The test-only config copy lets you verify config-wrapping behavior (e.g., structure of state_dict).
 - **Weight conversion helpers**: Write test-only helpers for any weight format differences between HF and custom (e.g., RoPE de-interleaving, stacked-to-per-expert MoE weights, gate weight key remapping). For full-model tests, prefer using `load_state_dict` pre-hooks already registered on the custom model.
 
 **Numerical comparison:** For equivalence tests comparing custom ops against HF reference, use the shared `assert_rmse_close` utility from `_model_test_utils`:
@@ -190,6 +198,10 @@ Run with full num layers. The generation should be coherent in step 2.
   - "changed attention backend to torch_mha"
   - "fixed weight loading hooks"
 
+The model is run via:
+```bash
+CUDA_VISIBLE_DEVICES=<SELECTED_GPUS> python examples/auto_deploy/build_and_run_ad.py --model <MODEL-ID> --use-registry
+```
 The `ad-run-agent` will determine the required `world_size` from the registry, check GPU availability via `nvidia-smi`, select free GPUs, and wait if not enough are available.
 
 The ad-run-agent will build+run the model, check generation quality, archive logs, and update its worklog.
@@ -233,7 +245,7 @@ When you post a PR, you **MUST** include:
 ```
 python examples/auto_deploy/build_and_run_ad.py --model <MODEL-ID> --use-registry
 ```
-3. A detailed pytest command for the unit tests you added so they can be run by the reviewer as well.
+3. A detailed pytest command for the unit tests you added so they can be run by the reviewer as well. Make sure you have run this pytest command on the latest commit that you are pushing, and include these results in the PR.
 
 ## Key Gotchas
 - **Canonical ops first:** Always use `torch.ops.auto_deploy.torch_*` canonical ops whenever one exists for the operation. This is how AD knows what to optimize. Writing manual attention, MoE, RoPE, or normalization in plain PyTorch instead of using the canonical op will prevent AD transforms from working.