feat: make litellm dependency optional#973
Open
aholka wants to merge 25 commits into
Open
Conversation
…ed after merge with main" This reverts commit d77e3c6.
…oper cleanup of weaviate resources between tests
…er, remove LiteLLM dependency from VertexAI embedder
…l used in examples, remove useless TYPE_CHECKING
aholka
changed the title
Contributor
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 46c33ba Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…hub + fix another mypy issue
aholka
force-pushed
the
967-feat-make-litellm-dependency-optional
branch
from
5fc31ab to
2a07b8e
Compare
…67-feat-make-litellm-dependency-optional # Conflicts: # examples/core/prompt/multimodal_with_pdf.py # examples/evaluate/dataset-generator/config/generate.yaml # packages/ragbits-chat/pyproject.toml # packages/ragbits/pyproject.toml # pyproject.toml # uv.lock
Collaborator
puzzle-solver
left a comment
puzzle-solver
left a comment
There was a problem hiding this comment.
Overall looks good, but we need to make sure that this is the direction we want to go in. I would suggest leaving the LiteLLM dependency to not make a breaking change yet and independently add OpenAI/Anthropic/Google providers. Also, we need to make sure that the implementations of them are correct, including token counting and cost estimation, which right now is broken.
Eventually, I think adding something like RagbitsLLM that routes between the three providers would be cool (similarly to LiteLLM). LiteLLM would be a fallback to other models if needed.
| """ | ||
| Dataclass that represents available call options for the OpenAIEmbedder client. | ||
| Each of them is described in the | ||
| [OpenAI API documentation](https://platform.openai.com/docs/api-reference/embeddings/create). |
| class OpenAILLMOptions(LLMOptions): | ||
| """ | ||
| Dataclass that represents all available LLM call options for the OpenAI client. | ||
| Each of them is described in the [OpenAI API documentation](https://platform.openai.com/docs/api-reference/chat). |
Collaborator
There was a problem hiding this comment.
404 not found. I think openai made their documentation a little bit more confusing recently.
| """ | ||
|
|
||
| frequency_penalty: float | None | NotGiven = NOT_GIVEN | ||
| max_tokens: int | None | NotGiven = NOT_GIVEN |
Collaborator
There was a problem hiding this comment.
max_tokens here are declared again, even though base LLMOptions already has it. I know that it's not a scope of this PR, but other classes don't provide it, so we need some consistency. I think some set of parameters can be potentially moved to the base LLMOptions class (also temperature and top_p). Or we can ditch the LLMOptions class completely, since it doesn't really provide much value.
| """ | ||
| Returns 0.0 — cost estimation is not bundled; use OpenAI usage dashboard for billing details. | ||
| """ | ||
| return 0.0 |
Collaborator
There was a problem hiding this comment.
I mean, this is not too great. We probably shouldn't return 0 for a cost just like that if we actually have this method.
Collaborator
There was a problem hiding this comment.
This is also the case for Gemini and Anthropic
| return sum(len(enc.encode(str(msg.get("content") or ""))) for msg in prompt.chat) | ||
| except KeyError: | ||
| pass | ||
| return sum(len(str(msg.get("content") or "")) for msg in prompt.chat) |
Collaborator
There was a problem hiding this comment.
Not a good fallback, we should probably add tiktoken to dependencies explicitly when the user wants to use openai, otherwise they will silently get completely wrong token counts (number of characters is probably a few times higher than number of tokens)
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
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.
Breaking change: LiteLLM becomes an optional dependency.
New LLM implementations: OpenAILLM, AnthropicLLM, GeminiLLM
New embedder implementations: OpenAIEmbedder, GeminiEmbedder.
note: this branch contains changes from add-docker-containers-for-integration-testing branch which hasn't been merged to main yet