Skip to content

Contribution baseten.co / engine builder: better dataloader in trt-llm - #137

Closed
michaelfeil wants to merge 2 commits into
NVIDIA:mainfrom
michaelfeil:mf/contrib-baseten-batch-size-fwd
Closed

Contribution baseten.co / engine builder: better dataloader in trt-llm#137
michaelfeil wants to merge 2 commits into
NVIDIA:mainfrom
michaelfeil:mf/contrib-baseten-batch-size-fwd

Conversation

@michaelfeil

@michaelfeil michaelfeil commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

Contribution by baseten.co (running the https://docs.baseten.co/performance/engine-builder-overview engine-builder product)

This PR:

  • guarantees OOM-free quantization as fallback, by sequentially lowering the batch size - its a superset of the current "lower by 2 once" low_mem_mode.

In my opinion, this PR should not be required. However the following circumstances have made it necessary:

def quantize_and_export(*,
                        model_dir,
                        device,
                        calib_dataset: str
                        batch_size: int,

at the time of setting this

  • to determine the batch_size, we need to load the model. In trt-llm, the model is typically not loaded. Loading the model twice (e.g. 70B llama) would likely lead to a long loading time or an OOM if the model is not freed. A size based on the model safetensors is not possible, as this depends entirely on.g. if the weights will be loaded from fp32 -> fp16

How to solve it:

  • tensorrt-llm should use the modelopt workflow - this would not fix the problem, as it does not use batch_size = get_max_batch_size(model, max_sample_length) function yet.
  • this PR needs to be implemented in trt-llm or here, best both.

@michaelfeil michaelfeil changed the title Contribution: better dataloader in trt-llm Contribution baseten.co / engine builder: better dataloader in trt-llm Feb 20, 2025
@michaelfeil

Copy link
Copy Markdown
Contributor Author

@kevalmorabia97 I see you have reviewed previous PRs here, I'd be glad if you would also review this one.

@kevalmorabia97

Copy link
Copy Markdown
Collaborator

@michaelfeil thanks for your contribution. Let us review this internally and test in our CI. If it looks good we would include this in one of the upcoming releases and give you credit for contribution on the commit

@michaelfeil

Copy link
Copy Markdown
Contributor Author

@kevalmorabia97 Any update on this? - no need to credit me, just would love to have this or a similar logic in main.

@cjluo-nv

cjluo-nv commented Mar 13, 2025

Copy link
Copy Markdown
Collaborator

Thanks for the contribution @michaelfeil I will merge this to the internal branch and include it in 0.27 release planned for April 3

@cjluo-nv cjluo-nv closed this Mar 13, 2025
@michaelfeil

Copy link
Copy Markdown
Contributor Author

Thanks!

kevalmorabia97 added a commit that referenced this pull request Apr 3, 2025
Co-Authored-By: michaelfeil <63565275+michaelfeil@users.noreply.github.com>
@kevalmorabia97

Copy link
Copy Markdown
Collaborator

@michaelfeil your contribution is now merged and added you as a co-author of the commit.
Thank you for your contribution. We are working on making more frequent github updates soon.

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