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

Add options for growable memory and single state buffers #104

Merged
merged 7 commits into from
Nov 15, 2023

Conversation

Tabrizian
Copy link
Member

No description provided.

@Tabrizian Tabrizian changed the title Imant single state Add options for growable memory and single state buffers Nov 6, 2023
Comment on lines 1398 to 1399
//@@ Currently, this option only applies for implicit state that uses CUDA and
//@@ use_single_buffer must be enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, we force the implicit state memory used in growable block manager to be GPU memory for now. Maybe the wording here could be clearer that regardless of what the user does, this will use GPU memory for now, even if they request other memory types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why wouldn't we error if they try different types? Just double checking - we don't silently switch preferences, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why wouldn't we error if they try different types?

What the user specifies is just "preferences" (i.e., there is no guarantee that Triton will satisfy the request). The same pattern exists in TRITONBackend_OutputBuffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the question is which options take precedence - does the model config or the API call to get the buffer - will discuss offline

protobuf/model_config.proto Outdated Show resolved Hide resolved
protobuf/model_config.proto Outdated Show resolved Hide resolved
protobuf/model_config.proto Outdated Show resolved Hide resolved

//@@ .. cpp:var:: bool use_growable_memory
//@@
//@@ The optional field to allow an implicit state buffer to grow the
Copy link
Contributor

@nnshah1 nnshah1 Nov 11, 2023

Choose a reason for hiding this comment

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

Does this need to be a user visible option? Wondering if we could always grow memory if using CUDA memory in the implementation.

@Tabrizian Tabrizian requested review from nnshah1 November 13, 2023 06:25
rmccorm4
rmccorm4 previously approved these changes Nov 13, 2023
nnshah1
nnshah1 previously approved these changes Nov 13, 2023
@Tabrizian Tabrizian dismissed stale reviews from nnshah1 and rmccorm4 via c8e8c8b November 14, 2023 05:48
@Tabrizian Tabrizian requested a review from rmccorm4 November 15, 2023 16:25
//@@ This option is only available for CUDA memory and requires enabling
//@@ use_same_buffer_for_input_output. When using this option,
//@@ StateBuffer call will always return CUDA memory even if CPU memory
//@@ is provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//@@ is provided.
//@@ is requested.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch fixed!

@Tabrizian Tabrizian merged commit 9f8c873 into main Nov 15, 2023
1 check passed
@Tabrizian Tabrizian deleted the imant-single-state branch November 15, 2023 16:35
Tabrizian added a commit that referenced this pull request Nov 15, 2023
* Add same input/output bstate buffer option

* Add an option for using GrowableMemory

* Review comments

* Format

* Review comments

* Review comment

* Fix description
mc-nv pushed a commit that referenced this pull request Nov 15, 2023
* Add same input/output bstate buffer option

* Add an option for using GrowableMemory

* Review comments

* Format

* Review comments

* Review comment

* Fix description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants