Map user-input session_id to internal session_id to maintain session identity#4523
Map user-input session_id to internal session_id to maintain session identity#4523lvhan028 wants to merge 4 commits intoInternLM:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a mapping layer between user-provided session_id values and internally managed session IDs, aiming to preserve session identity while preventing direct access to internal session IDs.
Changes:
- Add
VariableInterface.user_session_id_mappluscreate_session()/find_session()to translate user session IDs to internal session IDs. - Update OpenAI endpoints to use
create_session()and update abort behavior to look up sessions via the user→internal mapping. - Extend
SessionManager.get()with acreate_if_not_existsflag and adjust session-id generation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
lmdeploy/serve/openai/api_server.py |
Adds user→internal session-id mapping, updates session creation/lookup, and changes abort behavior to use mapped sessions. |
lmdeploy/serve/managers/session_manager.py |
Adjusts session-id generation and adds optional “lookup-only” behavior to SessionManager.get(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def map_user_session_id(self, user_session_id: int) -> int: | ||
| """Map a user_session_id to a session_id.""" | ||
| if user_session_id in self.user_session_id_map: | ||
| raise ValueError(f'User session id {user_session_id} already exists') | ||
| session_id = next(self.session_id_generator) | ||
| self.user_session_id_map[user_session_id] = session_id | ||
| self.session_id_map[session_id] = user_session_id | ||
| return session_id |
There was a problem hiding this comment.
map_user_session_id() currently raises if user_session_id is already present. Given the API server expects the same user-provided session_id to be reusable across requests (and create_session() calls this on every request), this should be idempotent: return the existing mapped session_id when present, and only allocate a new internal session_id when absent. Otherwise, repeated calls with the same user_session_id will crash the request flow.
| if VariableInterface.tool_parser is not None: | ||
| request = VariableInterface.tool_parser.adjust_request(request) | ||
| session = VariableInterface.get_session(request.session_id) | ||
| session = VariableInterface.create_session(request.session_id) |
There was a problem hiding this comment.
chat_completions_v1 now passes a mapped/internal session via create_session(). Later in this handler the response id is derived from session.session_id, which will now be the internal id (not the user-provided session_id) and can effectively expose internal ids to clients. Consider switching response ids to use the user-provided session id (or a separate request UUID) so internal ids remain an implementation detail, and keep behavior consistent with /v1/completions.
| session_id = session_mgr.map_user_session_id(user_session_id) | ||
| logger.info(f'created session {session_id} for user_session_id {user_session_id}') |
There was a problem hiding this comment.
create_session() calls session_mgr.map_user_session_id(user_session_id) unconditionally. As implemented, map_user_session_id raises if the user_session_id already exists, so repeated requests (or interactive continuation) with the same session_id will fail with an unhandled ValueError (500) instead of reusing the existing session mapping. Make the mapping lookup idempotent (return the existing mapped session_id when present) or check user_session_id_map first and only create a new mapping when it doesn't exist.
| session_id = session_mgr.map_user_session_id(user_session_id) | |
| logger.info(f'created session {session_id} for user_session_id {user_session_id}') | |
| session_id = session_mgr.user_session_id_map.get(user_session_id) | |
| if session_id is None: | |
| session_id = session_mgr.map_user_session_id(user_session_id) | |
| logger.info(f'created session {session_id} for user_session_id {user_session_id}') |
No description provided.