Remove modelRouter and add model_providers concept#224
Remove modelRouter and add model_providers concept#224philipph-askui wants to merge 56 commits intomainfrom
Conversation
…del store BREAKING CHANGE: Removed ModelRouter and ModelRegistry classes. Users must now use direct model injection.
…raises an Error when executing from cache)
…nd AskUiInferenceLocateApi
programminx-askui
left a comment
There was a problem hiding this comment.
Hello @philipph-askui ,
I started to review the code. General remarks:
- Changes are to big to review. -> We need to test this heavly
- Creating new files, instead of rename/move files -> we don't know which code was already reviewed, which code is new.
Overall it is going in the right direction.
I've reviewed only view files, so you can start working on it. A deeper review is outstanding.
Co-authored-by: Dominik Klotz <105296959+programminx-askui@users.noreply.github.com>
Co-authored-by: Dominik Klotz <105296959+programminx-askui@users.noreply.github.com>
Co-authored-by: Dominik Klotz <105296959+programminx-askui@users.noreply.github.com>
Co-authored-by: Dominik Klotz <105296959+programminx-askui@users.noreply.github.com>
…ve clear separation between Model, Client, messagesAPI and provider
|
I added a new optional dependency group "tracing", that contains all the OTEL packages. |
| validate_by_alias=True, | ||
| ) | ||
|
|
||
| base_name: str = Field(alias="name", description="Name of the tool") |
There was a problem hiding this comment.
Does this renaming has influence of any seralized data? e.g. Caching?
There was a problem hiding this comment.
no, should still work as the tool name provided by to the model should not change
| self._model_id_value = model_id | ||
| self._askui_settings = askui_settings or AskUiInferenceApiSettings() | ||
| self._model_id = model_id | ||
| self._injected_client = client |
src/askui/models/google/get_model.py
Outdated
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| _MAX_FILE_SIZE_BYTES = 20 * 1024 * 1024 |
There was a problem hiding this comment.
Where is the limit coming from? Can we link to the API docs from google in a comment? Otherwise I wouldn't check the file size.
There was a problem hiding this comment.
I think I found it here: https://ai.google.dev/gemini-api/docs/file-input-methods
According to a quick research, the limit was increased to 50MB recently. Hence, I updated the limit in the code.
| if len(data) > _MAX_FILE_SIZE_BYTES: | ||
| err_msg = ( | ||
| f"PDF file size exceeds the limit of {_MAX_FILE_SIZE_BYTES} bytes." | ||
| ) | ||
| raise ValueError(err_msg) |
There was a problem hiding this comment.
Duplicated code. please wrap this an a function check_file_size()
There was a problem hiding this comment.
the error messages are different, hence all we could wrap in a function here is the if comparison
| from askui.models.shared.tools import ToolCollection | ||
|
|
||
|
|
||
| def _to_openai_messages(messages: list[MessageParam]) -> list[dict[str, Any]]: # noqa: C901 |
There was a problem hiding this comment.
Should I review this?
I would expect here an integration tests. Which coveres all message types.
| """OpenAICompatibleMessagesApi — | ||
| MessagesApi implementation for OpenAI-compatible endpoints.""" | ||
|
|
||
| import json as json_lib | ||
| from typing import Any | ||
|
|
||
| from openai import OpenAI | ||
| from typing_extensions import override | ||
|
|
||
| from askui.models.shared.agent_message_param import ( | ||
| Base64ImageSourceParam, | ||
| ContentBlockParam, | ||
| ImageBlockParam, | ||
| MessageParam, | ||
| TextBlockParam, | ||
| ThinkingConfigParam, | ||
| ToolChoiceParam, | ||
| ToolResultBlockParam, |
There was a problem hiding this comment.
Please delete OpenAI Act integration. -> Let's move this to another issue.
…tic `provider_options`
[Edited] PR Description
To get a quick idea of the new API, I added an example under examples/model_providers.py that you can run
Note: You will need an anthropic API Key for that
This PR:
Summary
Key Changes
Further Changes
removes chat-api related code as it was deprecated
removes UI-TARS related code as it was deprecated
Breaking Changes