Conversation
Move build declarations, narrow request types, and native generation handlers into each decoder family. Preserve sampling defaults, legacy unsupported-input checks, and StableLM mixed-precision controls while exercising owner commands from existing E2E helpers. Python CPU owner tests passed: 151 passed and 21 unselected E2E cases skipped. Existing model graph definitions and numerical assertions are unchanged. Signed-off-by: yifeif <277870278+yifeif-nv@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/TensorRT-Model-Connect/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (81)
Included review availability: Your plan provides up to 12 included reviews per hour; 2 remain after this review. 📝 SummaryA detailed high-level summary could not be generated for this review. Here is an overview derived from the analyzed file changes:
WalkthroughTen model families add family-specific build and generate commands. Build handlers validate requests and publish bundles. Native adapters parse generation settings, load text-generation tasks, and return generated text, token IDs, and timings. Tests cover both command paths and isolated runtime staging. ChangesFamily build commands
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant FamilyCLI
participant FamilyAdapter
participant load_task
participant TextGenerationTask
FamilyCLI->>FamilyAdapter: pass generate request JSON
FamilyAdapter->>load_task: load family bundle and runtime settings
load_task->>TextGenerationTask: return text-generation task
FamilyAdapter->>TextGenerationTask: generate prompt with config
TextGenerationTask-->>FamilyAdapter: return text, token IDs, and timings
FamilyAdapter-->>FamilyCLI: write JSON result or error
Merge Risk: ⚪ Minimal · up to The new family commands add build and generation paths, and the checked default generation limit matches the existing behavior. No concrete merge-blocking issue is established. 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 10.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 256 functions across 50 files. (31 skipped: 20 unsupported, 11 over the file limit.) Comment |
github-actions
Bot
removed
the
run-internal-ci
yifeif-nv
marked this pull request as ready for review
September 22, 2026 22:40
This branch has not been deployed
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
1 participant
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.
Background
Follow up #1310 by moving ten existing families' CLI definitions and execution into their owning directories. New options can then be added without changing the public parser or shared build request.
Exit Criteria
Implementation
Families:
bloom,codegen,falcon,gpt2,gpt_neo,gpt_neox,opt,stablelm,starcoder2,xglm.Adds
buildandgenerate, lazy Python build handlers and native adapter targets. Existing family runtime interfaces are reused inside owner handlers. Model graphs and build policies remain in their existing owners. Existing flat CLI entry points remain compatible; public Task ABI, bundle format and dependencies are unchanged.A generic CI test fixture now derives its runtime files from the selected family declaration. The existing missing-testcase assertion is unchanged; missing descriptors and adapters remain errors. This is the only change outside the ten family directories.
Change categories
Validation
Commands and Results
Partial remote CI on
64984d36a103ff7debcf1249d100152a7e2c37d5:Stable Community CI— passed;Dev Community CI— failed;TRTMC Internal CI / Automated premerge gate— passed. Dev GPU validation remains incomplete because of a provisioning infrastructure failure; a normal CI retry is pending.CI_BASE_REF=4b9cc2b0f259e8959e1a5c0e996506e60d7101b5 python3 -m tools.ci pipeline source-quality: passed, including 299 architecture and CI contract tests.Owner CPU suite: 151 passed / 21 existing selector skips:
python3 -m pytest families/bloom/tests families/codegen/tests families/falcon/tests families/gpt2/tests families/gpt_neo/tests families/gpt_neox/tests families/opt/tests families/stablelm/tests families/starcoder2/tests families/xglm/tests -m "not gpu and not trt" -q -p no:cacheprovidercmake --build /work/build --parallel 8andctest --test-dir /work/build --output-on-failure --label-exclude gpu: a local integration tree containing all five independent migration batches passed 268 tests / 7 expected no-device skips, including this batch's 10 native CLI handler tests.python3 -m build --no-isolation --wheel --outdir /work/wheels -Cbuild-dir=/work/wheel-build .: the same integration tree built a real wheel; archive/installed validators passed for all 128 model families. Installed-only probes passed owner build dispatch for all 50 migrated families, 120 offline help commands and 70 native adapter family guards.Hardware, Environment, and Revisions
Head:
64984d36a103ff7debcf1249d100152a7e2c37d5. Base:4b9cc2b0f259e8959e1a5c0e996506e60d7101b5. Local Linux/aarch64 CPU-only validation used TensorRT 11.1.0.106 and Torch 2.12.0+cu130. Synthetic runtime fixtures and probe bundles were used for CLI integration; the skipped tests retain their existing explicit GPU/E2E selectors.Not Run / Remaining Gaps
Fresh checkpoint GPU inference, numerical/performance qualification and TensorRT-RTX execution were not run locally. The passing remote results above cover their configured checks on this head. Dev GPU validation remains incomplete, so merge readiness is pending.
Contributor Self-Review
Reviewed family ownership, supported parameters/defaults, old Python-call compatibility, native input/output contracts and failure behavior. Publication is gated on an independent review of outgoing code, commit metadata, PR text and CI output paths.
Notes For Future Readers
This is batch 5 of five independent ten-family migrations on the same base; no batch depends on another. Review
cli.json,cli.py, nativecli.cpp, then owner tests. Remaining families can follow the same pattern. Remove the shared legacy CLI/request only after migration is complete.Risk level
Ten families gain a new command path and adapter artifacts. Explicit defaults, strict rejection, owner tests and installed-package checks cover the integration boundary; they do not replace model qualification.