[None][fix] SpecDecOneEngineForCausalLM: accept optional hidden_size/vocab_size for composite configs - #16762
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesSpeculative model dimension overrides
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/models/modeling_speculative.py (1)
1897-1901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required annotations to the changed functions.
tensorrt_llm/_torch/models/modeling_speculative.py#L1897-L1901: useint | Nonefor the new parameters and add-> None.tests/unittest/_torch/modeling/test_modeling_speculative.py#L157-L157: annotate helper arguments and itsMagicMockreturn type.tests/unittest/_torch/modeling/test_modeling_speculative.py#L170-L170: add-> None.tests/unittest/_torch/modeling/test_modeling_speculative.py#L185-L185: add-> None.tests/unittest/_torch/modeling/test_modeling_speculative.py#L205-L205: add-> None.Suggested change
- hidden_size: Optional[int] = None, - vocab_size: Optional[int] = None): + hidden_size: int | None = None, + vocab_size: int | None = None) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/models/modeling_speculative.py` around lines 1897 - 1901, Annotate the changed __init__ in tensorrt_llm/_torch/models/modeling_speculative.py:1897-1901 with int | None for hidden_size and vocab_size and -> None. In tests/unittest/_torch/modeling/test_modeling_speculative.py:157, annotate the helper arguments and MagicMock return type; add -> None to the helpers at lines 170, 185, and 205.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_speculative.py`:
- Around line 1897-1901: Annotate the changed __init__ in
tensorrt_llm/_torch/models/modeling_speculative.py:1897-1901 with int | None for
hidden_size and vocab_size and -> None. In
tests/unittest/_torch/modeling/test_modeling_speculative.py:157, annotate the
helper arguments and MagicMock return type; add -> None to the helpers at lines
170, 185, and 205.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 13000814-63f8-498c-b643-6e438b9049ba
📒 Files selected for processing (2)
tensorrt_llm/_torch/models/modeling_speculative.pytests/unittest/_torch/modeling/test_modeling_speculative.py
|
/bot run |
|
PR_Github #61586 [ run ] triggered by Bot. Commit: |
|
PR_Github #61586 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61971 [ run ] triggered by Bot. Commit: |
|
PR_Github #61971 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62002 [ run ] triggered by Bot. Commit: |
|
PR_Github #62002 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62027 [ run ] triggered by Bot. Commit: |
|
PR_Github #62027 [ run ] completed with state
|
…vocab_size for composite configs Composite model configs (e.g. vision-language wrappers) may store hidden_size and vocab_size inside a nested text_config rather than at the top level of pretrained_config. The two new optional parameters let callers pass the text-config values explicitly; when omitted the existing behaviour (read from pretrained_config) is preserved. Regression test: three mock-based cases covering the default path, the explicit-override path, and the no-top-level-attrs path. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
…on, annotations - Tests use real ModelConfig/PretrainedConfig instead of MagicMock configs - Construct SpecDecOneEngineForCausalLM directly instead of __new__/__init__ - Fold return_value into patch(); expected values held in variables - Annotate the new constructor params (int | None, -> None) and tests Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
DecoderModelForCausalLM is built on the PostInitCaller metaclass, which calls __post_init__/__pp_init__ right after __init__ returns. The new SpecDecOneEngineForCausalLM tests mocked only __init__, so the hooks ran on a half-initialized object and raised AttributeError: 'SpecDecOneEngineForCausalLM' object has no attribute 'model_config'. Stub the metaclass hooks as well. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
/bot run |
ade0e53 to
84e392e
Compare
|
PR_Github #62042 [ run ] triggered by Bot. Commit: |
|
PR_Github #62042 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62690 [ run ] triggered by Bot. Commit: |
|
PR_Github #62690 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #62781 [ run ] triggered by Bot. Commit: |
|
PR_Github #62782 [ run ] triggered by Bot. Commit: |
|
PR_Github #62781 [ run ] completed with state |
|
PR_Github #62782 [ run ] completed with state |
Description
SpecDecOneEngineForCausalLM.__init__previously readhidden_sizeandvocab_sizeunconditionally frommodel_config.pretrained_config, which breaks for composite model configs (e.g. vision-language wrappers) where these fields live in a nestedtext_configrather than at the top level.Two new optional constructor parameters (
hidden_size,vocab_size) let callers pass the text-config values explicitly. When omitted, the existing behaviour is preserved — values are read frompretrained_configas before — so all current callers are unaffected.Root cause
For models whose config wraps a
text_config(e.g.Qwen3NextForCausalLM, which subclassesSpecDecOneEngineForCausalLM), the top-level config does not expose these attributes, causingAttributeErroron construction.Fix
Make both parameters optional with
Nonedefaults; fall through topretrained_configonly when not provided explicitly:Test Coverage
Added three mock-based unit tests to
tests/unittest/_torch/modeling/test_modeling_speculative.py(no GPU required, <1s):test_specdec_one_engine_reads_from_pretrained_config: default path passes values frompretrained_configtest_specdec_one_engine_accepts_explicit_sizes: composite-config path uses caller-supplied sizes whenpretrained_configlacks the attributestest_specdec_one_engine_explicit_overrides_pretrained_config: explicit args take precedence even whenpretrained_confighas valuesDev Engineer Review
SpecDecOneEngineForCausalLM.__init__(tensorrt_llm/_torch/models/modeling_speculative.py) to accept optionalhidden_size: int | Noneandvocab_size: int | None.hidden_size/vocab_sizeareNone, they fall back tomodel_config.pretrained_config.hidden_size/.vocab_size; provided values are forwarded toDecoderModelForCausalLM.__init__viasuper(...)(preserving prior behavior while enabling overrides for composite configs).pretrained_config,pretrained_configomits them,PostInitCallerbehavior by stubbingDecoderModelForCausalLM.__init__,__post_init__, and__pp_init__.QA Engineer Review
tests/unittest/_torch/modeling/test_modeling_speculative.py_init_specdec_with_mocked_base(model_config, **kwargs)test_specdec_one_engine_reads_from_pretrained_configtest_specdec_one_engine_accepts_explicit_sizestest_specdec_one_engine_explicit_overrides_pretrained_configunittest/_torch/modeling -k "modeling_speculative"(e.g.,tests/integration/test_lists/test-db/l0_a30.yml), which should exercise the updatedtest_modeling_speculative.pyunit tests.tests/integration/test_lists//tests/qa//tests/test-db/.