jeff3071 commented on code in PR #72002:
URL: https://github.com/apache/airflow/pull/72002#discussion_r3999050151
##########
providers/common/ai/src/airflow/providers/common/ai/hooks/llamaindex.py:
##########
@@ -179,7 +186,18 @@ def get_embedding_model(self) -> BaseEmbedding:
extra_key="embed_model",
kind="embedding",
)
- return OpenAIEmbedding(model=model_id, **self._connection_kwargs(conn))
+ connection_kwargs = self._connection_kwargs(conn)
+ overridden_keys = sorted(self.embedding_kwargs.keys() &
connection_kwargs.keys())
+ if overridden_keys:
+ self.log.warning("Connection parameters override embedding_kwargs
values: %s", overridden_keys)
+ kwargs = {**self.embedding_kwargs, **connection_kwargs}
+ supported_kwargs =
set(inspect.signature(OpenAIEmbedding.__init__).parameters) | set(
+ OpenAIEmbedding.model_fields
+ )
+ unsupported_keys = sorted(self.embedding_kwargs.keys() -
supported_kwargs)
Review Comment:
I chose to reserve `{"model", "model_name"}` and raise an error, following
the principle that "embedding_kwargs should not override parameters".
The same issue exists in `LangChainHook`.
`embed_model="openai:text-embedding-3-small"` with
`embedding_kwargs={"provider": "cohere"}` would select `cohere` provider.
Solution: reserve {"model", "model_name", "provider"} are reserved and will
raise an error.
---
`OpenAIEmbedding` accepts `additional_kwargs` and forwards them to
`embeddings.create`.
Passing `embedding_kwargs={"additional_kwargs": {"model": ...}}` already
raises an error downstream, but the message does not identify
`embedding_kwargs` as the cause.
Solution: checks this case and raises a clearer error.
> `LangChainHook` does not apply this guard because the final
`client.create` parameters are provider-specific and cannot be reliably
enumerated here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
