[None][chore] Update AD onboard skill and reviewer agent#241
Merged
govind-ramnarayan merged 10 commits intoMar 12, 2026
Merged
Conversation
- 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>
| 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. **Always use actual HF classes — never write 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. |
Author
There was a problem hiding this comment.
Let's make this "always" more relaxed. Let's say always use actual HF classes if they exist.
| **HF Reference Strategy:** Equivalence tests compare our custom implementation against the HF reference with identical weights and inputs. **Always use actual HF classes — never write 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`**: load them from the HF cache downloaded in Phase 0. Use `importlib.util` or insert the cache snapshot directory into `sys.path` to import the model's `modeling_*.py` directly (e.g., `~/.cache/huggingface/hub/.../modeling_{name}.py`). Do **NOT** copy or rewrite class definitions into the test file — the test must import from actual HF source. If the HF source cannot be loaded at all, skip with `pytest.skip("HF reference unavailable")`. |
Author
There was a problem hiding this comment.
Let's keep the old text here; the above should hopefully be enough. Let's add that we should strongly prefer using HF reference directly, and try to make tests pass with this.
|
|
||
| ## Phase 9 — AutoDeploy End-to-End Run | ||
|
|
||
| ### ⚠️ MANDATORY: You MUST use `build_and_run_ad.py --use-registry` EXACTLY AS-IS ⚠️ |
| 2. Fix the issue (model code, **registry config yaml**, weight hooks, etc.) | ||
| 2. Fix the issue (model code, registry config yaml, weight hooks, etc.) | ||
| 3. Re-invoke the ad-run-agent with an updated description reflecting the change (e.g., "retry after fixing RoPE scaling in config") | ||
| 4. **Always re-run with `--use-registry`.** Never bypass the registry. |
Author
There was a problem hiding this comment.
Keep this (instruction to always use with registry)
|
|
||
| If the run **fails** or produces **bad generation**: | ||
| 1. Read the ad-run-agent's worklog and log file to understand the error | ||
| 2. Fix the issue (model code, **registry config yaml**, weight hooks, etc.) |
| branch `feat/paperclip_maximizer`. Then, ask the user to provide feedback on the PR and wait for the | ||
| user to get back to you when the feedback has been posted. Then continue iterating according to the | ||
| user's feedback. For any comment or other post, please prepend your message with "[AGENT]" so that it is clear that this was a coding agent posting the comment. | ||
| When you post a PR, you **MUST** include: |
Author
There was a problem hiding this comment.
The existing is fine here.
| - **Reuse config classes:** Import from `transformers` or load from checkpoint whenever possible. Only bundle a config class if it truly doesn't exist anywhere. | ||
| - **Assert `position_ids`:** Always assert `position_ids is not None` — it is a required input, never optional. | ||
| - **Self-contained files only**: Never import from other AD custom models. Each `modeling_{name}.py` is a standalone translation from HF source. | ||
| - **RoPE cos/sin: slice ONCE, not per layer.** `_ad_` prefix for RoPE buffers. `RotaryEmbedding.forward(x, position_ids)` MUST slice by `position_ids` once and return pre-sliced `(cos, sin)`. Pass those tensors to all layers. NEVER pass `position_ids` through to each layer/attention forward to re-index — that is redundant compute that bloats the exported graph. See Phase 2 for the full pattern. |
| | 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 HF cache imports only); no hardcoded local/temp path imports. HF reference classes must be imported from actual HF source — either from `transformers` directly, or from the HF cache via `importlib`/`sys.path`. **No standalone class definitions** that mirror the model architecture (e.g., hand-written `HFAttention`, `HFDecoderLayer`, `RefModel`). 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, flag as FAIL. | Inspect all class definitions and imports in the test file. | |
Author
There was a problem hiding this comment.
maybe relax is to say no standalone class definitions if a viable HF alternative exists.
- 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>
Author
|
[AGENT] Addressed all review feedback:
|
| | # | 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. | | ||
| | BB3 | 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. | |
| ```bash | ||
| CUDA_VISIBLE_DEVICES=<SELECTED_GPUS> python examples/auto_deploy/build_and_run_ad.py --model <MODEL-ID> --use-registry | ||
| ``` | ||
| The `--use-registry` flag automatically resolves the model's config from `models.yaml`, so no manual `--args.yaml-extra` is needed. 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. |
Author
There was a problem hiding this comment.
First sentence here not needed.
| **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`**: strongly prefer loading them from the HF cache downloaded in Phase 0. Use `importlib.util` or insert the cache snapshot directory into `sys.path` to import the model's `modeling_*.py` directly (e.g., `~/.cache/huggingface/hub/.../modeling_{name}.py`). Try to make tests pass with this approach first. If the HF source truly cannot be loaded at test time, copy the minimal module definitions from the HF `modeling_*.py` source into the test file as standalone reference classes — but only as a last resort. **Important**: make sure the copy is minimal and strictly faithful to the HF implementation only. Do NOT tweak the functionality of the reference. |
Author
There was a problem hiding this comment.
Don't need to be overly specific; can omit this line:
Use importlib.util or insert the cache snapshot directory into sys.path to import the model's modeling_*.py directly (e.g., ~/.cache/huggingface/hub/.../modeling_{name}.py). Try to make tests pass with this approach first.
- 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>
Author
|
[AGENT] Addressed the new comments:
|
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>
…ase 4 Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
| |---|-------|---------------| | ||
| | 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` (e.g., a speculative-decoding draft model like Eagle whose config has no upstream HF class), or (b) the model code requires config attributes not present in the checkpoint's `config.json` and not available in the standard HF class. Both conditions must be confirmed; if neither applies, the custom class is a FAIL. | |
Author
There was a problem hiding this comment.
Let's remove the example about Eagle. Instead include the heuristic that if E2E can work without the custom config class because AutoConfig will pick up the config, then the custom class is not needed.
| | 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 HF cache imports only); no hardcoded local/temp path imports. HF reference classes must be imported from actual HF source — either from `transformers` directly, or from the HF cache via `importlib`/`sys.path`. No standalone class definitions that mirror the model architecture (e.g., hand-written `HFAttention`, `HFDecoderLayer`, `RefModel`) when a viable HF alternative exists. 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 a viable HF reference class was available, flag as FAIL. | Inspect all class definitions and imports in the test file. | |
There was a problem hiding this comment.
or from the HF cache via
importlib/sys.path.
This is a hack IMO and should be avoid I think. HF cache paths are not dependable for unit tests..
…or custom config Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
…_pretrained check Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
| **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`**: strongly prefer loading them from the HF cache downloaded in Phase 0. If the HF source truly cannot be loaded at test time, copy the minimal module definitions from the HF `modeling_*.py` source into the test file as standalone reference classes — but only as a last resort. **Important**: make sure the copy is minimal and strictly faithful to the HF implementation only. Do NOT tweak the functionality of the reference. |
There was a problem hiding this comment.
I'm not sure if we can let it use from HF cache. This unit test needs to run in CI and it may not find the HF cache there. So we might want a self contained, faithful implementation of HF code that can be checked in...
…or 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>
| 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 (e.g. for `config_class` or type annotations). |
Author
There was a problem hiding this comment.
Even for config_class it is not needed - I believe there are examples where we set it to None because we don't need a custom config and can't access the existing one in this file. A reason would be if there are variables needed for the model that are not in the loaded config, and not defaulted in the code.
| **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., nested `llm_config`, `vision_config` for multi-modal models) and confirm that vision weight keys load correctly. |
Author
There was a problem hiding this comment.
The part about vision weight keys loading correctly at the end is overly specific, as is the bit about multi-modal models. Maybe just say it lets you verify config-wrapping behavior (e.g. structure of state_dict), something like that.
… 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>
- 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>
govind-ramnarayan
force-pushed
the
gramnarayan/skill-workflow-updates
branch
from
bebc009 to
f779130
Compare
govind-ramnarayan
merged commit ea2ca73
into
feat/paperclip_maximizer
5 of 7 checks passed
bmarimuthu-nv
pushed a commit
that referenced
this pull request
Mar 13, 2026
* [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>
bmarimuthu-nv
pushed a commit
that referenced
this pull request
Mar 13, 2026
* [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>
bmarimuthu-nv
pushed a commit
that referenced
this pull request
Mar 14, 2026
* [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: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Add E2E sanity check note for custom config class in Phase 4 Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: replace Eagle example with E2E heuristic for custom config Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: remove E2E heuristic, keep AutoConfig.from_pretrained check Signed-off-by: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> --------- Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
bmarimuthu-nv
pushed a commit
that referenced
this pull request
Mar 18, 2026
* [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: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Add E2E sanity check note for custom config class in Phase 4 Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: replace Eagle example with E2E heuristic for custom config Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: remove E2E heuristic, keep AutoConfig.from_pretrained check Signed-off-by: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> --------- Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
bmarimuthu-nv
pushed a commit
that referenced
this pull request
Mar 25, 2026
* [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: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Add E2E sanity check note for custom config class in Phase 4 Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: replace Eagle example with E2E heuristic for custom config Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: remove E2E heuristic, keep AutoConfig.from_pretrained check Signed-off-by: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> --------- Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
bmarimuthu-nv
pushed a commit
that referenced
this pull request
Apr 1, 2026
* [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: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Add E2E sanity check note for custom config class in Phase 4 Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: replace Eagle example with E2E heuristic for custom config Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> * [None][chore] Reviewer B2: remove E2E heuristic, keep AutoConfig.from_pretrained check Signed-off-by: gramnarayan <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: gramnarayan <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: gramnarayan <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: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> --------- Signed-off-by: gramnarayan <105831528+govind-ramnarayan@users.noreply.github.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
2 participants
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.
Summary
importlib/sys.path. No standalone HF-like class definitions allowed in tests (they defeat the purpose of reference comparisons).nn.Modulehierarchy required, vision weight assertions required in tests.transformersand (b) required config attributes are missing from standard HF class.AutoConfig.from_pretrained(trust_remote_code=True), so explicitly importing the config class is rarely needed.⚠️ MANDATORYwarning blocks; consolidated--use-registryguidance into a single clear statement. Simplified summary report format.Test plan
🤖 Generated with Claude Code