[wip] Custom session handlers #25
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… SAGEMAKER_ as prefix.
…ds into toggle-sticky-routing
…is None but request is a session request, raise 400 error - change session api transform's transform_request to never return request field in output (previously used to pass session_manager - change handler code to only take raw_request as a parameter and use new utility function get_session_manager - add new integration tests for expected errors when sessions is disabled
…ds into toggle-sticky-routing
…eate/close session apis
|
Logic looks good to me in general. I want to understand the CX before we move further. Can you add docs/examples to help understand the CX ? |
…ds into toggle-sticky-routing Signed-off-by: Zuyi Zhao <[email protected]>
…container-standards into custom-session-handlers Signed-off-by: Zuyi Zhao <[email protected]>
…ds into custom-session-handlers Signed-off-by: Zuyi Zhao <[email protected]>
- Fix session ID injection logic to work with default session manager - Changed conditional from elif to separate if statements - Session ID now properly injected into request body when session_id_path is specified - Validation and injection can now happen independently - Update unit tests to match refactored transform API - Remove import of non-existent _validate_session_if_present function - Update test signatures to match _process_session_request parameters - Add missing SessionRequest import - Replace outdated test logic with current implementation - Add integration tests for session_id_path injection feature - Test session ID injection into flat and nested request body paths - Test injection with multiple requests and different sessions - Verify existing body fields are preserved during injection
- Rename session_id_path to request_session_id_path in stateful_session_manager - Rename session_id_path to response_session_id_path in register handlers - Add docstring clarifying request_session_id_path usage - Provide default content messages for create/close session handlers - Remove specific engine references (vLLM, TGI) from documentation - Delete obsolete test_registration.py - Improve integration tests
…ds into custom-session-handlers
Lokiiiiii
requested changes
Dec 8, 2025
Lokiiiiii
left a comment
Lokiiiiii
left a comment
There was a problem hiding this comment.
Summarizing -
- The parameter names for the handlers are un-intuitive. I recommend renaming it to be obvious in terms of the functionality or the input the developer needs to provide -
path_to_session_id_in_incoming_requestor something similar. - The documentation has inconsistent ordering of decorators. This will cause issues when overriding handlers.
- We need to improve the defaults and documentation needs to indicate defaults for handler arguments
python/model_hosting_container_standards/sagemaker/sessions/CUSTOM_HANDLERS.md
Outdated
Show resolved
Hide resolved
python/model_hosting_container_standards/sagemaker/sessions/CUSTOM_HANDLERS.md
Show resolved
Hide resolved
python/model_hosting_container_standards/sagemaker/sessions/CUSTOM_HANDLERS.md
Outdated
Show resolved
Hide resolved
Lokiiiiii
reviewed
Dec 8, 2025
python/model_hosting_container_standards/sagemaker/sessions/CUSTOM_HANDLERS.md
Outdated
Show resolved
Hide resolved
…ds into custom-session-handlers
…ation API Improve the developer experience for registering custom session handlers by introducing a cleaner API with better parameter naming and adding a build_session_request_shape helper to consolidate request shape logic. This makes it easier to understand how session IDs are injected and provides better conflict detection through logging.
…stom session handlers Rename parameters to better distinguish between engine request and response paths, making it clearer what data flows where. Make engine_request_session_id_path optional for create_session while keeping it required for close_session (must know which session to close). Changes: - Rename request_session_id_path → engine_request_session_id_path - Rename response_session_id_path → engine_response_session_id_path - Make engine_request_session_id_path optional in create handler - Add validation to require engine_request_session_id_path in close handler - Expand docstrings with detailed explanations, examples, and limitations - Update all tests and documentation to use new parameter names
Lokiiiiii
reviewed
Dec 10, 2025
python/model_hosting_container_standards/common/fastapi/utils.py
Outdated
Show resolved
Hide resolved
…ds into custom-session-handlers
…ds into custom-session-handlers
…uest when target key is specified for session id injection to support create_parent
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.