[docs] feat: update model support skill with encapsulation guidance#3313
[docs] feat: update model support skill with encapsulation guidance#3313
Conversation
Clarify that LLMs should use bridge-only pattern (no provider file) and add a new "Encapsulating model-specific layers" section with three strategies for handling custom architectural blocks: local mapping subclasses, bridge hook overrides, and VLM-only custom providers. Also expand the decision tree for custom layer handling. Signed-off-by: Chen Cui <chcui@nvidia.com>
📝 WalkthroughWalkthroughDocumentation update for model support skill that removes the requirement for provider files in most LLM cases, revises VLM workflow guidance, introduces a new section on encapsulating model-specific layers, expands strategy guidance with concrete examples, and updates decision tree routing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/adding-model-support/SKILL.md (1)
99-111:⚠️ Potential issue | 🟡 MinorUse a consistent
modeling/modellingdirectory naming conventionLine 99 uses
modelling_<model>/while Line 111 usesmodeling_<model>.py. If both aren’t intentionally distinct conventions, this is likely to cause copy-paste setup errors.Suggested doc fix
-└── modelling_<model>/ # If using Megatron vision encoder +└── modeling_<model>/ # If using Megatron vision encoder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/adding-model-support/SKILL.md` around lines 99 - 111, The docs mix British and American spelling for the module directory/file names causing potential copy-paste errors; pick one convention and make names consistent (e.g., change `modelling_<model>/` to `modeling_<model>/` or vice versa) so the directory name and the file `modeling_<model>.py` align, and update any references in the same section to use the chosen symbol (`modeling_<model>` or `modelling_<model>`) uniformly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/adding-model-support/SKILL.md`:
- Around line 119-120: The sentence overstates that GPTModelProvider covers "all
LLMs"; update the documentation text to clarify that super().provider_bridge()
returns a provider determined by PROVIDER_CLASS and that GPTModelProvider is
sufficient only for GPT-style LLM implementations — for non-GPT providers
authors should verify the returned provider type from provider_bridge() and
implement provider-specific logic or subclassing as needed. Reference
GPTModelProvider, provider_bridge(), and PROVIDER_CLASS in the revised sentence
to make the behavior explicit.
---
Outside diff comments:
In `@skills/adding-model-support/SKILL.md`:
- Around line 99-111: The docs mix British and American spelling for the module
directory/file names causing potential copy-paste errors; pick one convention
and make names consistent (e.g., change `modelling_<model>/` to
`modeling_<model>/` or vice versa) so the directory name and the file
`modeling_<model>.py` align, and update any references in the same section to
use the chosen symbol (`modeling_<model>` or `modelling_<model>`) uniformly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 062513ca-81d8-4b31-8ebc-2c19d9e0f41f
📒 Files selected for processing (1)
skills/adding-model-support/SKILL.md
- Fix inconsistent directory naming: modelling_<model>/ → modeling_<model>/ - Soften provider guarantee: clarify that super().provider_bridge() returns a stock provider (not necessarily GPTModelProvider) via PROVIDER_CLASS - Apply same fix to Strategy 3 section for consistency Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Summary
GPTModelProviderreturned bysuper().provider_bridge()is sufficient for all LLMsTest plan
Summary by CodeRabbit