Haystack llm and embedding wrapper #1901
Conversation
sahusiddharth
requested review from
shahules786 and
jjmachan
and removed request for
shahules786
dosubot
bot
added
the
size:L
| try: | ||
| from haystack_experimental.core import AsyncPipeline | ||
| except ImportError: | ||
| raise ImportError( | ||
| "haystack-experimental is not installed. Please install it using `pip install haystack-experimental==0.4.0`." | ||
| ) | ||
| try: | ||
| from haystack.components.embedders import ( # type: ignore | ||
| AzureOpenAITextEmbedder, | ||
| HuggingFaceAPITextEmbedder, | ||
| OpenAITextEmbedder, | ||
| SentenceTransformersTextEmbedder, | ||
| ) | ||
| except ImportError: | ||
| raise ImportError( | ||
| "pip install haystack-ai is not installed. Please install it using `pip install pip install haystack-ai`." | ||
| ) |
There was a problem hiding this comment.
this code still makes haystack a direct dependency for running Ragas
There was a problem hiding this comment.
I want you to write test that
- removes the haystack lib installed in the dev
- tries and imports the module
make sure it is failing
then refer to https://github.com/BerriAI/litellm/blob/5c2839a744814712b9fdeb1dec6609333743f8e9/litellm/llms/bedrock/base_aws_llm.py#L266
here you can see that they only import boto3 when they need it.
write something like that for haystack and make it pass
There was a problem hiding this comment.
the failing test before the implementation is important. In order to test this we have to remove the haystack module already installed from python environment
tests/unit/test_simple.py
Outdated
| # Replace the built-in import function with our mock | ||
| monkeypatch.setattr(builtins, "__import__", mocked_import) | ||
|
|
||
| with pytest.raises(ImportError, match="Haystack is not installed"): | ||
| from ragas.llms import HaystackLLMWrapper | ||
|
|
||
| HaystackLLMWrapper(haystack_generator=None) | ||
|
|
There was a problem hiding this comment.
the test should check that we can import other modules too (like LangchainLLMWrapper, without any issues- that part is more important.
What I'm afraid is that because of some import Haystack will become a dependency
There was a problem hiding this comment.
I have added the test to ensures that:
- No ImportError is raised for non-Haystack imports when haystack is missing.
- The Haystack wrapper does raise ImportError in that scenario.
No description provided.