Skip to content

[https://nvbugs/6245317][test] set Harmony tiktoken env for GPT-OSS disagg - #14935

Merged
dongfengy merged 1 commit into
NVIDIA:mainfrom
dongfengy:fix-gptoss-harmony-tiktoken-env
Jun 8, 2026
Merged

[https://nvbugs/6245317][test] set Harmony tiktoken env for GPT-OSS disagg#14935
dongfengy merged 1 commit into
NVIDIA:mainfrom
dongfengy:fix-gptoss-harmony-tiktoken-env

Conversation

@dongfengy

@dongfengy dongfengy commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Without this, tiktoken will try to download online and sometimes fail with
[05/26/2026-11:55:37] [TRT-LLM] [E] [serve] Error in harmony chat completion: %s error downloading or loading vocab file: failed to download or load vocab file

Summary by CodeRabbit

  • Tests
    • Improved test environment configuration for disaggregated model testing to ensure proper resource handling and isolation.

Description

Test Coverage

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

@dongfengy
dongfengy force-pushed the fix-gptoss-harmony-tiktoken-env branch from 74f8516 to eff1338 Compare June 4, 2026 04:43
@dongfengy

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@dongfengy
dongfengy marked this pull request as ready for review June 4, 2026 04:43
@dongfengy
dongfengy requested a review from a team as a code owner June 4, 2026 04:43
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0a64bea4-3e77-4b4d-8a7a-6de4fe0ca6a3

📥 Commits

Reviewing files that changed from the base of the PR and between 846d0f4 and eff1338.

📒 Files selected for processing (1)
  • tests/integration/defs/disaggregated/test_disaggregated.py

📝 Walkthrough

Walkthrough

A test in the disaggregated integration test suite now sets up tiktoken cache environment variables before running the test. The environment is copied from llm_venv._new_env and modified with TIKTOKEN_RS_CACHE_DIR and TIKTOKEN_ENCODINGS_BASE pointing to a tiktoken_vocab directory under llm_models_root()/datasets, then passed to run_disaggregated_test.

Changes

Tiktoken Environment Variables for Disaggregated Test

Layer / File(s) Summary
Tiktoken cache environment configuration
tests/integration/defs/disaggregated/test_disaggregated.py
The test prepares a modified environment by copying llm_venv._new_env, computing a tiktoken_vocab path under llm_models_root()/datasets, exporting that path via TIKTOKEN_RS_CACHE_DIR and TIKTOKEN_ENCODINGS_BASE, then passing the modified env to run_disaggregated_test.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description contains only the problem statement about tiktoken errors but lacks required template sections like Description, Test Coverage, and detailed explanation of the solution. Add a clear Description section explaining the issue and solution, and list the relevant test(s) in Test Coverage section as required by the template.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: setting Harmony tiktoken environment variables for a GPT-OSS disaggregated test to prevent online vocab file downloads.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51970 [ run ] triggered by Bot. Commit: eff1338 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51970 [ run ] completed with state FAILURE. Commit: eff1338
/LLM/main/L0_MergeRequest_PR pipeline #41319 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@dongfengy
dongfengy force-pushed the fix-gptoss-harmony-tiktoken-env branch from eff1338 to e787532 Compare June 4, 2026 17:17
@dongfengy

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52122 [ run ] triggered by Bot. Commit: e787532 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52122 [ run ] completed with state FAILURE. Commit: e787532
/LLM/main/L0_MergeRequest_PR pipeline #41447 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@dongfengy

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52136 [ run ] triggered by Bot. Commit: e787532 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52136 [ run ] completed with state SUCCESS. Commit: e787532
/LLM/main/L0_MergeRequest_PR pipeline #41460 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

Signed-off-by: Dongfeng Yu <dongfengy@nvidia.com>
@dongfengy
dongfengy force-pushed the fix-gptoss-harmony-tiktoken-env branch from e787532 to cc549e8 Compare June 5, 2026 16:42
@dongfengy

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52383 [ run ] triggered by Bot. Commit: cc549e8 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52383 [ run ] completed with state SUCCESS. Commit: cc549e8
/LLM/main/L0_MergeRequest_PR pipeline #41678 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@dongfengy

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52443 [ run ] triggered by Bot. Commit: cc549e8 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #52443 [ run ] completed with state SUCCESS. Commit: cc549e8
/LLM/main/L0_MergeRequest_PR pipeline #41736 completed with status: 'SUCCESS'

CI Report

Link to invocation

@dongfengy
dongfengy merged commit 86f33e6 into NVIDIA:main Jun 8, 2026
7 checks passed
2ez4bz pushed a commit to 2ez4bz/TensorRT-LLM that referenced this pull request Jun 8, 2026
…isagg (NVIDIA#14935)

Signed-off-by: Dongfeng Yu <dongfengy@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants