TensorRT-LLM icon indicating copy to clipboard operation
TensorRT-LLM copied to clipboard

chore: Only read cfg json once

Open LetsGoFir opened this issue 11 months ago • 1 comments

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 avatar Feb 05 '25 11:02 LetsGoFir

@LetsGoFir pls rebase this MR to the latest main branch. @Superjomn can you help review MR when it becomes ready?

Thanks June

juney-nvidia avatar Mar 24 '25 05:03 juney-nvidia

@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

karljang avatar Sep 26 '25 21:09 karljang

📝 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 refactor
tensorrt_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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 26 '25 21:09 coderabbitai[bot]