Skip to content

[TRTLLM-3456] Speculation: Draft Target in new FW - #4558

Merged
mikeiovine merged 5 commits into
NVIDIA:mainfrom
IzzyPutterman:iputterman/draft-target-pyt-2-model
Jun 16, 2025
Merged

[TRTLLM-3456] Speculation: Draft Target in new FW#4558
mikeiovine merged 5 commits into
NVIDIA:mainfrom
IzzyPutterman:iputterman/draft-target-pyt-2-model

Conversation

@IzzyPutterman

Copy link
Copy Markdown
Contributor

No description provided.

@IzzyPutterman
IzzyPutterman requested a review from a team as a code owner May 22, 2025 06:12
@IzzyPutterman
IzzyPutterman requested review from byshiue and hlu1 May 22, 2025 06:12

@mikeiovine mikeiovine left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Let's add a test?

Comment thread examples/pytorch/quickstart_advanced.py Outdated
Comment thread tensorrt_llm/_torch/speculative/draft_target.py Outdated
Comment thread tensorrt_llm/_torch/speculative/interface.py Outdated
@mikeiovine

Copy link
Copy Markdown
Collaborator

By the way, I realized that this was a bit too specific to eagle 3: https://github.com/NVIDIA/TensorRT-LLM/blob/main/tensorrt_llm/_torch/pyexecutor/py_executor.py#L1817

You don't have to re-compute KV cache for verified tokens in this case; that's only a thing in MTP/eagle. We can make it optional. No need to fix this now since it's a performance thing, won't hurt correctness.

@IzzyPutterman
IzzyPutterman requested a review from a team as a code owner May 22, 2025 16:15
@Funatiq
Funatiq self-requested a review May 27, 2025 11:08
Comment thread tensorrt_llm/_torch/speculative/utils.py Outdated
Comment thread tests/unittest/_torch/speculative/test_draft_target.py Outdated
Comment thread tests/unittest/_torch/speculative/test_draft_target.py Outdated
Comment thread tests/unittest/_torch/speculative/test_draft_target.py Outdated
Comment thread tensorrt_llm/_torch/pyexecutor/py_executor_creator.py Outdated
@IzzyPutterman
IzzyPutterman requested a review from a team as a code owner May 27, 2025 15:30
@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

Some variance in output is expected (I think) specifically when a draft token is from the prefill portion.

@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

Apparently I have found an edge case in the formatting :)

@Funatiq

Funatiq commented May 30, 2025

Copy link
Copy Markdown
Collaborator

Apparently I have found an edge case in the formatting :)

I guess it was a conflict between yapf and isort or something similar? Could you please disable the formatting locally instead of changing the config file? Something like:

# isort: off
...
# isort: on

or

# yapf: disable
...
# yapf: enable

Comment thread tensorrt_llm/_torch/pyexecutor/sampler.py Outdated
Comment thread tensorrt_llm/_torch/speculative/interface.py Outdated
Comment thread tests/unittest/_torch/speculative/test_draft_target.py Outdated
@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

Depends on these fixes: #4807

@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

1 similar comment
@mikeiovine

Copy link
Copy Markdown
Collaborator

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8320 [ run ] triggered by Bot

@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8320 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #6027 completed with status: 'FAILURE'

@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

1 similar comment
@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

@IzzyPutterman
IzzyPutterman force-pushed the iputterman/draft-target-pyt-2-model branch from 5ce0535 to 340922c Compare June 11, 2025 15:48
@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

1 similar comment
@Funatiq

Funatiq commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8517 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8517 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #6176 completed with status: 'FAILURE'

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8568 [ run ] completed with state ABORTED

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8569 [ kill ] completed with state SUCCESS
Successfully killed previous jobs for commit a1cff6c

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8570 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8570 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #6216 completed with status: 'FAILURE'

@IzzyPutterman
IzzyPutterman force-pushed the iputterman/draft-target-pyt-2-model branch from a1cff6c to e22b7a4 Compare June 12, 2025 16:06
@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8704 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8704 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #6312 completed with status: 'FAILURE'

@Funatiq

Funatiq commented Jun 13, 2025

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8774 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #8774 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #6371 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

@IzzyPutterman
IzzyPutterman force-pushed the iputterman/draft-target-pyt-2-model branch from e22b7a4 to 73f069a Compare June 14, 2025 19:41
Izzy Putterman added 5 commits June 16, 2025 07:49
Signed-off-by: Izzy Putterman <iputterman@nvidia.com>
Signed-off-by: Izzy Putterman <iputterman@nvidia.com>
Signed-off-by: Izzy Putterman <iputterman@nvidia.com>
Signed-off-by: Izzy Putterman <iputterman@nvidia.com>
Signed-off-by: Izzy Putterman <iputterman@nvidia.com>
@IzzyPutterman
IzzyPutterman force-pushed the iputterman/draft-target-pyt-2-model branch from 73f069a to d6d087d Compare June 16, 2025 14:49
@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot skip

@github-actions

Copy link
Copy Markdown

GitHub Bot Help

/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...

Provide a user friendly way for developers to interact with a Jenkins server.

Run /bot [-h|--help] to print this help message.

See details below for each supported subcommand.

Details

run [--disable-fail-fast --skip-test --stage-list "A10-1, xxx" --gpu-type "A30, H100_PCIe" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-[Post-Merge]-1, xxx"]

Launch build/test pipelines. All previously running jobs will be killed.

--disable-fail-fast (OPTIONAL) : Disable fail fast on build/tests/infra failures.

--skip-test (OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.

--stage-list "A10-1, xxx" (OPTIONAL) : Only run the specified test stages. Examples: "A10-1, xxx". Note: Does NOT update GitHub check status.

--gpu-type "A30, H100_PCIe" (OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.

--only-multi-gpu-test (OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.

--disable-multi-gpu-test (OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.

--add-multi-gpu-test (OPTIONAL) : Force run the multi-GPU tests. Will also run L0 pre-merge pipeline.

--post-merge (OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.

--extra-stage "H100_PCIe-[Post-Merge]-1, xxx" (OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-[Post-Merge]-1, xxx".

kill

kill

Kill all running builds associated with pull request.

skip

skip --comment COMMENT

Skip testing for latest commit on pull request. --comment "Reason for skipping build/test" is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

reuse-pipeline

reuse-pipeline

Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot skip "No major changes since last CI pass, just rebase"

@github-actions

Copy link
Copy Markdown

GitHub Bot Help

/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...

Provide a user friendly way for developers to interact with a Jenkins server.

Run /bot [-h|--help] to print this help message.

See details below for each supported subcommand.

Details

run [--disable-fail-fast --skip-test --stage-list "A10-1, xxx" --gpu-type "A30, H100_PCIe" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-[Post-Merge]-1, xxx"]

Launch build/test pipelines. All previously running jobs will be killed.

--disable-fail-fast (OPTIONAL) : Disable fail fast on build/tests/infra failures.

--skip-test (OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.

--stage-list "A10-1, xxx" (OPTIONAL) : Only run the specified test stages. Examples: "A10-1, xxx". Note: Does NOT update GitHub check status.

--gpu-type "A30, H100_PCIe" (OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.

--only-multi-gpu-test (OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.

--disable-multi-gpu-test (OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.

--add-multi-gpu-test (OPTIONAL) : Force run the multi-GPU tests. Will also run L0 pre-merge pipeline.

--post-merge (OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.

--extra-stage "H100_PCIe-[Post-Merge]-1, xxx" (OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-[Post-Merge]-1, xxx".

kill

kill

Kill all running builds associated with pull request.

skip

skip --comment COMMENT

Skip testing for latest commit on pull request. --comment "Reason for skipping build/test" is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

reuse-pipeline

reuse-pipeline

Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

@IzzyPutterman

Copy link
Copy Markdown
Contributor Author

/bot skip --comment "No major changes since last CI pass, just rebase"

@mikeiovine
mikeiovine enabled auto-merge (squash) June 16, 2025 18:11
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #9054 [ skip ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #9054 [ skip ] completed with state SUCCESS
Skipping testing for commit d6d087d

@mikeiovine
mikeiovine merged commit e607768 into NVIDIA:main Jun 16, 2025
3 checks passed
@Funatiq Funatiq changed the title Speculation: Draft Target in new FW [TRTLLM-3456] Speculation: Draft Target in new FW Jun 20, 2025
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.

5 participants