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

[Trt-llm] always perform verbose dump of defaults for forward compatibility #1338

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

michaelfeil
Copy link
Contributor

@michaelfeil michaelfeil commented Jan 24, 2025

🚀 What

Background:

  1. a config of (paged_kv_cache=False, enable_chunked_context=True) will lead to errors in briton.
  2. if a user on 0.9.57 sets TrtLLMConfig(paged_kv_cache=False), this means only paged_kv_cache=False is written in the yaml. The default of the 0.9.57 was enable_chunked_context=False.
  3. The interpretation of the value enable_chunked_context is not clear from the user uploaded yaml. It depends on the truss version upstream.
  4. Now: The new default is changed to enable_chunked_context=True, on request of FDE

Issue:

  • Changing default will lead to an upload of invalid configs.
  • Not knowing what the original config of the user was, makes it dependent on the truss version currently used upstream.

Solutions:

  • To retain full compatibilit, model.yaml needs to be written out, and also migrated.
  • on upload, the yaml needs to be completed with the current backend version, and fully dumped.

💻 How

🔬 Testing

@michaelfeil michaelfeil changed the title [Trt-llm] full model dump if set [Trt-llm] always perform verbose dump of defaults for forward compatibility Jan 24, 2025
@bdubayah bdubayah self-requested a review January 24, 2025 22:25
Copy link
Contributor

@bdubayah bdubayah left a comment

Choose a reason for hiding this comment

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

Dumping the whole trtllm config makes sense to me. Looks like there's a test that is checking only set values are dumped that needs to be updated

@michaelfeil
Copy link
Contributor Author

Not merging this!

@michaelfeil michaelfeil marked this pull request as draft February 12, 2025 00:55
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.

2 participants