Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix wrong arg in Engine Building Command in docs/source/performance/perf-overview.md #2057

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RuibaiXu
Copy link

fix typo in Engine Building Command, --max_output_len should be --max_seq_len
otherwise report error with:
trtllm-build: error: unrecognized arguments: --max_output_len

fix wrong arg in Engine Building Command
no --max_output_len, should be --max_seq_len
@nv-guomingz
Copy link
Collaborator

Hi @RuibaiXu Thanks for your contribution to TRT-LLM, we'll merge you changes into internal code base.

@nv-guomingz nv-guomingz added documentation Improvements or additions to documentation Merged labels Jul 31, 2024
@RuibaiXu
Copy link
Author

RuibaiXu commented Aug 1, 2024

@nv-guomingz
sorry, i found that my PR code is not as same meaning as the original doc,
we should not just change --max_output_len to --max_seq_len
although there is no arg --max_output_len, but max_seq_len means max_input_len+max_output_len
so, as the original doc is --max_input_len 2048 --max_output_len 2048
the right doc after change shouled be --max_input_len 2048 --max_seq_len 4096
please change in your internal code base, thank you

@nv-guomingz
Copy link
Collaborator

@nv-guomingz sorry, i found that my PR code is not as same meaning as the original doc, we should not just change --max_output_len to --max_seq_len although there is no arg --max_output_len, but max_seq_len means max_input_len+max_output_len so, as the original doc is --max_input_len 2048 --max_output_len 2048 the right doc after change shouled be --max_input_len 2048 --max_seq_len 4096 please change in your internal code base, thank you

Got it. Thanks for reminding.

@kaiyux
Copy link
Member

kaiyux commented Oct 8, 2024

@RuibaiXu @nv-guomingz Please note that, the latest docs/source/performance/perf-overview.md is not using trtllm-build command anymore. (link)

the right doc after change shouled be --max_input_len 2048 --max_seq_len 4096

@RuibaiXu Just FYI - on the latest main branch, if you're using context fmha and remove input padding (both are enabled by default), then no need to specify max_input_len, as there is not such constrain now.

Thanks a lot for your support!

@kaiyux kaiyux removed the Merged label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants