-
Notifications
You must be signed in to change notification settings - Fork 98
LCORE-2311: Fix Skills #1972
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
LCORE-2311: Fix Skills #1972
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ | |
| from pydantic_ai import UnexpectedModelBehavior | ||
| from pydantic_ai._run_context import RunContext | ||
| from pydantic_ai._utils import PeekableAsyncStream, Unset, number_to_datetime | ||
| from pydantic_ai.messages import ModelMessage | ||
| from pydantic_ai.messages import ModelMessage, ModelResponse | ||
| from pydantic_ai.models import ( | ||
| ModelRequestParameters, | ||
| StreamedResponse, | ||
|
|
@@ -181,15 +181,79 @@ class LlamaStackResponsesModel(OpenAIResponsesModel): | |
| before the corresponding ``McpCall`` or ``ResponseFunctionToolCall`` item. | ||
| """ | ||
|
|
||
| async def request( # pylint: disable=unused-argument | ||
| self, | ||
| messages: list[ModelMessage], | ||
| model_settings: ModelSettings | None, | ||
| model_request_parameters: ModelRequestParameters, | ||
| run_context: RunContext[Any] | None = None, | ||
| ) -> Any: | ||
| """Non-streaming request with Llama Stack conversation continuation fix. | ||
|
|
||
| Llama Stack rejects requests containing both ``conversation`` and | ||
| ``previous_response_id``. On continuation turns (where a prior | ||
| ``ModelResponse`` exists), we trim messages to only the new input and | ||
| disable ``previous_response_id`` so that only ``conversation`` is sent. | ||
| This ensures all responses are persisted to the conversation. | ||
| """ | ||
| messages, model_settings = self._prepare_conversation_continuation( | ||
| messages, model_settings | ||
| ) | ||
| return await super().request(messages, model_settings, model_request_parameters) | ||
|
|
||
| def _prepare_conversation_continuation( | ||
| self, | ||
| messages: list[ModelMessage], | ||
| model_settings: ModelSettings | None, | ||
| ) -> tuple[list[ModelMessage], ModelSettings | None]: | ||
| """Trim messages and disable previous_response_id for conversation continuations. | ||
|
|
||
| Llama Stack rejects requests with both ``previous_response_id`` and | ||
| ``conversation``. When ``conversation`` is in ``extra_body`` and there's | ||
| already a ModelResponse in the history (a continuation turn), we: | ||
|
|
||
| 1. Trim messages to only those AFTER the last ModelResponse (new input only) | ||
| 2. Disable ``openai_previous_response_id`` so pydantic-ai won't resolve one | ||
|
|
||
| This means Llama Stack receives ``conversation`` (for persistence) plus only | ||
| the new input items. Llama Stack reconstructs prior history from the | ||
| conversation and appends the new input correctly. | ||
| """ | ||
|
Comment on lines
+191
to
+221
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Add the required Google docstring sections to the new/updated methods. The new As per coding guidelines, “Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes.” Also applies to: 253-266 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| if not model_settings or not isinstance(model_settings, dict): | ||
| return messages, model_settings | ||
|
|
||
| extra_body = model_settings.get("extra_body") | ||
| if not isinstance(extra_body, dict) or "conversation" not in extra_body: | ||
| return messages, model_settings | ||
|
|
||
| last_response_idx = None | ||
| for i in range(len(messages) - 1, -1, -1): | ||
| msg = messages[i] | ||
| if isinstance(msg, ModelResponse) and msg.provider_response_id: | ||
| last_response_idx = i | ||
| break | ||
|
|
||
| if last_response_idx is None: | ||
| return messages, model_settings | ||
|
|
||
| trimmed_messages = messages[last_response_idx + 1 :] | ||
|
|
||
| new_settings = dict(model_settings) | ||
| new_settings.pop("openai_previous_response_id", None) | ||
| return trimmed_messages, cast(ModelSettings, new_settings) | ||
|
|
||
| @asynccontextmanager | ||
| async def request_stream( | ||
| async def request_stream( # pylint: disable=unused-argument | ||
| self, | ||
| messages: list[ModelMessage], | ||
| model_settings: ModelSettings | None, | ||
| model_request_parameters: ModelRequestParameters, | ||
| run_context: RunContext[Any] | None = None, | ||
| ) -> AsyncIterator[StreamedResponse]: | ||
| """Request a streaming response, filtering Llama Stack-specific event quirks. | ||
| """Request a streaming response with Llama Stack compatibility fixes. | ||
|
|
||
| Applies the same conversation continuation handling as :meth:`request` | ||
| before calling the Responses API, then filters streaming tool-call events. | ||
|
|
||
| Args: | ||
| messages: Model messages for the request. | ||
|
|
@@ -201,10 +265,10 @@ async def request_stream( | |
| A StreamedResponse with the filtered event stream. | ||
| """ | ||
| check_allow_model_requests() | ||
| model_settings, model_request_parameters = self.prepare_request( | ||
| model_settings, | ||
| model_request_parameters, | ||
| messages, model_settings = self._prepare_conversation_continuation( | ||
| messages, model_settings | ||
| ) | ||
|
|
||
| model_settings_cast = cast(OpenAIResponsesModelSettings, model_settings or {}) | ||
| response = await self._responses_create( | ||
| messages, True, model_settings_cast, model_request_parameters | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -221,12 +221,29 @@ async def generate_agent_response( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| context.query_request.conversation_id is None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and bool(context.query_request.generate_topic_summary) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| topic_summary = await maybe_get_topic_summary( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| generate_topic_summary=should_generate_topic_summary, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| input_text=context.query_request.query, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client=context.client, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| model_id=responses_params.model, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| topic_summary = await maybe_get_topic_summary( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| generate_topic_summary=should_generate_topic_summary, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| input_text=context.query_request.query, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client=context.client, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| model_id=responses_params.model, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except HTTPException as exc: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Topic summary failed for request %s: %s", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| context.request_id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exc.detail, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| detail: dict[str, str] = exc.detail if isinstance(exc.detail, dict) else {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yield serialize_event( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorStreamPayload.create( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status_code=exc.status_code, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response=detail.get("response", "Internal server error"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cause=detail.get("cause", str(exc.detail)), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| media_type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+224
to
+246
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Don’t abort a completed stream on topic-summary failure. Returning here skips Suggested fix except HTTPException as exc:
logger.warning(
"Topic summary failed for request %s: %s",
context.request_id,
exc.detail,
)
- detail: dict[str, str] = exc.detail if isinstance(exc.detail, dict) else {}
- yield serialize_event(
- ErrorStreamPayload.create(
- status_code=exc.status_code,
- response=detail.get("response", "Internal server error"),
- cause=detail.get("cause", str(exc.detail)),
- ),
- media_type,
- )
- return
+ topic_summary = None📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info("Consuming tokens") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| consume_query_tokens( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| user_id=context.user_id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major
❓ Verification inconclusive
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 294
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 476
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 220
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 371
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 15057
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 5570
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 170
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 299
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 170
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 576
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 4722
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 414
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 834
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 631
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 1908
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 2202
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 170
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 525
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 4151
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 199
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 2860
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 284
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 170
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 1254
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 301
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 2035
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 1571
🏁 Script executed:
Repository: lightspeed-core/lightspeed-stack
Length of output: 231
Forward
run_contextwhen delegating to parent methods.Both
request()andrequest_stream()acceptrun_context, but neither forwards it when calling the parent implementation or helper methods, which could cause context loss if the baseOpenAIResponsesModelmethods consume it.🐛 Proposed fixes
For
request()at line 202:For
request_stream()at lines 273–274, if_responses_createacceptsrun_context:🤖 Prompt for AI Agents