chore: Only read cfg json once
Hello,
I noticed that the code here repeatedly reads the JSON file, which doesn’t seem very efficient. Additionally, in some cases, this file may be deleted because the path is automatically created. I made some changes; could you please check if they are reasonable?
@LetsGoFir pls rebase this MR to the latest main branch. @Superjomn can you help review MR when it becomes ready?
Thanks June
@LetsGoFir , thanks for your contribution, this PR looks still valid 👍 However could you address above DCO check failure, more details are available here: https://github.com/NVIDIA/TensorRT-LLM/blob/main/CONTRIBUTING.md#signing-your-work
📝 Walkthrough
Walkthrough
Adds a private helper to lazily read and cache config.json for engine build configuration. Refactors internal callers to use the helper instead of direct file I/O, notably in argument checking and sampling parameter preparation. No public API changes.
Changes
| Cohort / File(s) | Summary |
|---|---|
Config data loading refactortensorrt_llm/llmapi/llm.py |
Added _get_config_data(self) to lazily load/cache config.json under the model path. Updated _check_arguments and adjusted _prepare_sampling_params to use the new helper for a single source of config retrieval. No public signatures changed. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Caller as Caller
participant LLM as LLM instance
participant FS as File System
Caller->>LLM: _check_arguments(...)
Note over LLM: Needs engine build config
alt First access
LLM->>LLM: _get_config_data()
LLM->>FS: Read model_path/config.json
FS-->>LLM: JSON data
LLM->>LLM: Cache config data
else Cached
LLM->>LLM: _get_config_data() returns cached
end
LLM-->>Caller: Validation result
Caller->>LLM: _prepare_sampling_params(...)
LLM->>LLM: _get_config_data() (cached)
LLM-->>Caller: Sampling params
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description Check | ⚠️ Warning | The pull request description does not follow the repository template as it lacks the required sections for summary, detailed description, test coverage, and the PR checklist. | Please restructure the description to include the template headings: a summary, a Description section explaining the issue and solution, a Test Coverage section listing relevant tests, and the PR Checklist items. |
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title Check | ✅ Passed | The title succinctly describes the primary change by indicating that the configuration JSON file is only read once and uses the expected [None][chore] template. |
✨ Finishing touches
- [ ] 📝 Generate Docstrings
🧪 Generate unit tests
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
[!TIP]
👮 Agentic pre-merge checks are now available in preview!
Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
- Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
- Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.
Please see the documentation for more information.
Example:
reviews: pre_merge_checks: custom_checks: - name: "Undocumented Breaking Changes" mode: "warning" instructions: | Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.