Skip to content

fix: The constructor checks useDynamicTree but doesn’t validate dynamicTreeMaxTopK if set - #2977

Closed
A-transformer wants to merge 1 commit into
NVIDIA:mainfrom
A-transformer:mdynamictreemaxtopk_check
Closed

A-transformer wants to merge 1 commit into
NVIDIA:mainfrom
A-transformer:mdynamictreemaxtopk_check

Conversation

@A-transformer

Copy link
Copy Markdown

The check is tied to mDynamicTreeMaxTopK, which is only meaningful in the context of useDynamicTree. While the check could theoretically apply even when useDynamicTree is false, enforcing it here ensures it’s validated at construction time alongside related conditions.

@A-transformer
A-transformer force-pushed the mdynamictreemaxtopk_check branch from efe7290 to f81850b Compare March 21, 2025 17:51
@kaiyux
kaiyux requested review from byshiue and lfr-0531 March 24, 2025 02:39
@kaiyux

kaiyux commented Mar 24, 2025

Copy link
Copy Markdown
Member

/bot run

@niukuo

niukuo commented Mar 24, 2025

Copy link
Copy Markdown
Collaborator

PR_Github #212 [ run ] triggered by Bot

@niukuo

niukuo commented Mar 24, 2025

Copy link
Copy Markdown
Collaborator

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

@juney-nvidia juney-nvidia added the Community want to contribute PRs initiated from Community label Mar 24, 2025
@juney-nvidia

Copy link
Copy Markdown
Collaborator

@byshiue @lfr-0531 can you help review this MR?

Thanks
June

@juney-nvidia juney-nvidia changed the title The constructor checks useDynamicTree but doesn’t validate dynamicTreeMaxTopK if set fix: The constructor checks useDynamicTree but doesn’t validate dynamicTreeMaxTopK if set Mar 24, 2025

@lfr-0531 lfr-0531 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.

LGTM~

@lfr-0531

Copy link
Copy Markdown
Collaborator

/bot run

@niukuo

niukuo commented Mar 24, 2025

Copy link
Copy Markdown
Collaborator

PR_Github #242 [ run ] triggered by Bot

@niukuo

niukuo commented Mar 24, 2025

Copy link
Copy Markdown
Collaborator

PR_Github #242 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #239 completed with status: 'SUCCESS'

The check is tied to mDynamicTreeMaxTopK, which is only meaningful in the context of useDynamicTree. While the check could theoretically apply even when useDynamicTree is false, enforcing it here ensures it’s validated at construction time alongside related conditions.

Signed-off-by: root <root@us1.us-central1-f.c.pivotal-being-453505-m1.internal>
@kaiyux
kaiyux force-pushed the mdynamictreemaxtopk_check branch from 6112098 to e60f05d Compare March 24, 2025 09:07
@kaiyux

kaiyux commented Mar 24, 2025 •

Copy link
Copy Markdown
Member

@A-transformer The DCO check failed with the error message

Expected "A-transformer [cl5743590921@gmail.com](mailto:cl5743590921@gmail.com)", but got "root [root@us1.us-central1-f.c.pivotal-being-453505-m1.internal](mailto:root@us1.us-central1-f.c.pivotal-being-453505-m1.internal)"."

Can you help fix it?

@juney-nvidia juney-nvidia added the Community Engagement help/insights needed from community label Apr 12, 2025
@poweiw

poweiw commented Jun 5, 2025

Copy link
Copy Markdown
Contributor

Closing as no activities from requester for +10 days. Feel free to reopen 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community Engagement help/insights needed from community Community want to contribute PRs initiated from Community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants