You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ylnsnv (via GitHub)" <gi...@apache.org> on 2023/11/09 09:46:55 UTC
[PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
ylnsnv opened a new pull request, #35547:
URL: https://github.com/apache/airflow/pull/35547
## Summary
This PR refines the `OpenAIEmbeddingOperator` to accurately reflect and support the extended range of input types it can handle for generating embeddings ([by OpenAI official API](https://github.com/apache/airflow/assets/42522942/b15eef98-dcb9-42a4-b828-fba8fa9a47d1)). The operator, which previously accepted a string or a list of any type, now has improved type annotations and validation to handle strings, lists of strings, lists of integers, and lists of lists of integers more explicitly.
## Changes
- Modified type annotations `str | list[Any]` to be [`str | list[str] | list[int] | list[list[int]]`](https://github.com/apache/airflow/assets/42522942/b15eef98-dcb9-42a4-b828-fba8fa9a47d1)
- Refined input validation to ensure `input_text` is non-empty and matches one of the expected types.
- Expanded unit tests to cover the newly supported input types and to ensure the operator behaves as expected.
## Impact
The operator's enhanced input type support provides users with clearer guidance on what data can be passed for embedding generation. This update facilitates the use of the operator in a broader range of scenarios, such as processing numerical data or sequences of data, which is common in machine learning and NLP tasks.
## Tests
- Extended the test suite to include cases for each of the supported input types, ensuring robustness.
- Added negative test cases to verify that the operator raises exceptions for invalid inputs as expected.
With these changes, the `OpenAIEmbeddingOperator` becomes more intuitive and versatile, allowing for seamless integration into various data processing pipelines within Airflow environments.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1810504403
Hi @potiuk ,
I hope I'm not intruding or overstepping again, but I wanted to quickly check if there's anything else needed on my PR for it to be merge-ready.
Thanks for your time!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390719284
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
Relocated :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391283588
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
@utkarsharma2 , thanks for your response. Just to clarify, if we decide not to modify the test, does this imply that we should broaden the range of types accepted by the constructor's arguments to accommodate the dynamic runtime type (`Task[(), str]`)? I want to ensure we're on the same page regarding this aspect of handling dynamic types in the constructor.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1803713464
@potiuk hi! can you please review my changes in your free time? :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1807222276
LGTM but I'd love @utkarsharma2 to take a look as this is so fresh contribution :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390685637
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
+ raise AirflowException(
+ "The 'input_text' must be a non-empty string, list of strings, list of integers, or list of lists of integers."
+ )
+ self.log.info("Generating embeddings for the input text: %s", self.input_text)
embeddings = self.hook.create_embeddings(self.input_text, model=self.model, **self.embedding_kwargs)
- self.log.info("Embeddings: %s", embeddings)
+ self.log.info("Generated embeddings: %s", embeddings)
Review Comment:
same as thoughts as comment - https://github.com/apache/airflow/pull/35547/files#r1390682879
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391273148
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
@ylnsnv I think you are right, I haven't considered this point earlier. I'm happy to keep the check in the execute method. Please consider this point resolved.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390685136
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
we may want to move this check into __init__() since it would be a quicker response to the user specifying the wrong type.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1807585313
@ylnsnv Overall changes look good to me, just a few minor things. Also, can you mention the docs where you found the specific types accepted by OpenAI API? just curious :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1807596113
> @ylnsnv Overall changes look good to me, just a few minor things. Also, can you mention the docs where you found the specific types accepted by OpenAI API? just curious :)
First of all, thank you for your feedback!
To clarify, my initial observation about the `list[Any]` type being used in Airflow's code for OpenAI Operator input prompted me to investigate further.
I didn't find the specific type information in the OpenAI API documentation directly. Instead, I referred to the actual implementation of the OpenAI API in their codebase. There, I found a [line of code](https://github-production-user-asset-6210df.s3.amazonaws.com/42522942/281687312-b15eef98-dcb9-42a4-b828-fba8fa9a47d1.png) that clearly indicated the expected type. This discovery led me to open the PR to align Airflow's type indications with the actual implementation in the OpenAI API.
I'm glad to contribute and ensure our codebase is as accurate and up-to-date as possible. Thank you again for your thorough review and for pointing out areas that needed clarification. Your input is greatly valued! :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391317221
##########
tests/providers/openai/operators/test_openai.py:
##########
@@ -34,3 +36,30 @@ def test_execute_with_input_text():
embeddings = operator.execute(context)
assert embeddings == [1.0, 2.0, 3.0]
+
+
+def test_execute_with_invalid_input_empty_string():
Review Comment:
Maybe we can combine these 3 tests into a single parameterized test, WDYT?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1811115956
LGTM :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390686468
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
+ raise AirflowException(
+ "The 'input_text' must be a non-empty string, list of strings, list of integers, or list of lists of integers."
+ )
+ self.log.info("Generating embeddings for the input text: %s", self.input_text)
Review Comment:
I think you're 100% right, updating now! :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391343236
##########
tests/providers/openai/operators/test_openai.py:
##########
@@ -34,3 +36,30 @@ def test_execute_with_input_text():
embeddings = operator.execute(context)
assert embeddings == [1.0, 2.0, 3.0]
+
+
+def test_execute_with_invalid_input_empty_string():
Review Comment:
Done
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391336892
##########
tests/providers/openai/operators/test_openai.py:
##########
@@ -34,3 +36,30 @@ def test_execute_with_input_text():
embeddings = operator.execute(context)
assert embeddings == [1.0, 2.0, 3.0]
+
+
+def test_execute_with_invalid_input_empty_string():
Review Comment:
Definitely! great recommendation!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390719284
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
~~Relocated :)~~
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391317221
##########
tests/providers/openai/operators/test_openai.py:
##########
@@ -34,3 +36,30 @@ def test_execute_with_input_text():
embeddings = operator.execute(context)
assert embeddings == [1.0, 2.0, 3.0]
+
+
+def test_execute_with_invalid_input_empty_string():
Review Comment:
Maybe we can combine these 3 tests into a single parameterized test, WDYT? and also, add a test to check the positive test to check for all three valid types?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390689573
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
+ raise AirflowException(
+ "The 'input_text' must be a non-empty string, list of strings, list of integers, or list of lists of integers."
+ )
+ self.log.info("Generating embeddings for the input text: %s", self.input_text)
embeddings = self.hook.create_embeddings(self.input_text, model=self.model, **self.embedding_kwargs)
- self.log.info("Embeddings: %s", embeddings)
+ self.log.info("Generated embeddings: %s", embeddings)
Review Comment:
:+1:
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391315739
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
Nope, I don't think we have to handle the dynamic type explicitly, it's an internal construct of airflow and airflow is responsible for resolving templates, post resolution we can check the types `str or list[int], list[str]` which we are already doing. So overall this looks good.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390682879
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
+ raise AirflowException(
+ "The 'input_text' must be a non-empty string, list of strings, list of integers, or list of lists of integers."
+ )
+ self.log.info("Generating embeddings for the input text: %s", self.input_text)
Review Comment:
nit: we may not want to include the entire input text in logs since it can get very long. Instead, we can maybe just add the length input_text to signify it's not empty. WDYT?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391177321
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
So I've carefully considered your suggestion about relocating the validation to the `__init__` method. However, there's a potential issue with this approach, particularly in the context of how Airflow handles dynamic inputs through XComs and templating.
In the `example_openai_dag` within `airflow/tests/system/providers/openai/example_openai.py`, we have a scenario where the `input_text` for `OpenAIEmbeddingOperator` is dynamically determined at runtime. This is achieved by pulling a value from XCom that is produced by another task (`task_to_store_input_text_in_xcom`). If we place the validation in the `__init__` method, it would attempt to validate this dynamic reference (e.g., `{{ task_instance.xcom_pull(task_ids='task_to_store_input_text_in_xcom') }}`) as a literal value, which is not feasible since the actual value is only resolved during task execution.
This leads to a dilemma: if we keep the validation in `__init__`, we might reject valid DAG definitions that use dynamic inputs. On the other hand, placing the validation in the `execute` method allows for more flexibility, as it validates the actual runtime value after all templating and XCom pulls have been resolved.
I've adjusted the test to reflect this understanding, but I'm open to further discussion. The key question is whether we prioritize strict validation at DAG parsing time (with potential limitations on dynamic inputs) or more flexible, runtime validation that accommodates dynamic DAG patterns. Your insights on this trade-off, especially in the context of our current test cases and typical use cases for the `OpenAIEmbeddingOperator`, would be greatly appreciated.
Thanks again! :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #35547:
URL: https://github.com/apache/airflow/pull/35547
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1803726566
> > @potiuk hi! can you please review my changes in your free time? :)
>
> Generally - unless you have good reasons to know someone **SHOULD** review something we do not have a custom to call people in comments. ESPECIALLY if you opened your PR few hours before. People here contribute and review when they have free time and see where they can help. By calling me (or anyone else) directly - you also limit your options of getting review from other people, because they might think I am already involved and engaged with a PR, where I am not. So please avoid doing that unless you have very good reason to believe called person is absolutely necessary and SHOULD review the PR for whatever reason.
I apologize for any inconvenience my direct request may have caused. It was not my intention to limit the review opportunities by others or to impose unnecessarily. I appreciate the collaborative efforts here and will ensure to keep pr's open for the community to address collectively. Thank you for bringing this to my attention and sorry again!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1803719465
> @potiuk hi! can you please review my changes in your free time? :)
Generally - unless you have good reasons to know someone **SHOULD** review something we do not have a custom to call people in comments. ESPECIALLY if you opened your PR few hours before. People here contribute and review when they have free time and see where they can help. By calling me (or anyone else) directly - you also limit your options of getting review from other people, because they might think I am already involved and engaged with a PR, where I am not. So please avoid doing that unless you have very good reason to believe called person is absolutely necessary and SHOULD review the PR for whatever reason.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "ylnsnv (via GitHub)" <gi...@apache.org>.
ylnsnv commented on PR #35547:
URL: https://github.com/apache/airflow/pull/35547#issuecomment-1803580324
WIP, do not review yet.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391319485
##########
tests/providers/openai/operators/test_openai.py:
##########
@@ -34,3 +36,30 @@ def test_execute_with_input_text():
embeddings = operator.execute(context)
assert embeddings == [1.0, 2.0, 3.0]
+
+
+def test_execute_with_invalid_input_empty_string():
Review Comment:
Something like this - https://github.com/apache/airflow/blob/4cc98ba6648b4a350f9093c4e22ad6cff87ac5e4/tests/providers/amazon/aws/sensors/test_ecs.py#L198
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390691032
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
+ raise AirflowException(
Review Comment:
Also, this should be ValueError
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1390691032
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
+ raise AirflowException(
Review Comment:
I think this should be ValueError
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refine Type Handling in OpenAI Embedding Operator to Match OpenAI Typings [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #35547:
URL: https://github.com/apache/airflow/pull/35547#discussion_r1391709973
##########
airflow/providers/openai/operators/openai.py:
##########
@@ -31,41 +32,46 @@ class OpenAIEmbeddingOperator(BaseOperator):
"""
Operator that accepts input text to generate OpenAI embeddings using the specified model.
+ :param conn_id: The OpenAI connection ID to use.
+ :param input_text: The text to generate OpenAI embeddings for. This can be a string, a list of strings,
+ a list of integers, or a list of lists of integers.
+ :param model: The OpenAI model to be used for generating the embeddings.
+ :param embedding_kwargs: Additional keyword arguments to pass to the OpenAI `create_embeddings` method.
+
.. seealso::
For more information on how to use this operator, take a look at the guide:
:ref:`howto/operator:OpenAIEmbeddingOperator`
-
- :param conn_id: The OpenAI connection.
- :param input_text: The text to generate OpenAI embeddings on. Either input_text or input_callable
- should be provided.
- :param model: The OpenAI model to be used for generating the embeddings.
- :param embedding_kwargs: For possible option check
- .. seealso:: https://platform.openai.com/docs/api-reference/embeddings/create
+ For possible options for `embedding_kwargs`, see:
+ https://platform.openai.com/docs/api-reference/embeddings/create
"""
template_fields: Sequence[str] = ("input_text",)
def __init__(
self,
conn_id: str,
- input_text: str | list[Any],
+ input_text: str | list[str] | list[int] | list[list[int]],
model: str = "text-embedding-ada-002",
embedding_kwargs: dict | None = None,
**kwargs: Any,
):
- self.embedding_kwargs = embedding_kwargs or {}
super().__init__(**kwargs)
self.conn_id = conn_id
self.input_text = input_text
self.model = model
+ self.embedding_kwargs = embedding_kwargs or {}
@cached_property
def hook(self) -> OpenAIHook:
"""Return an instance of the OpenAIHook."""
return OpenAIHook(conn_id=self.conn_id)
def execute(self, context: Context) -> list[float]:
- self.log.info("Input text: %s", self.input_text)
+ if not self.input_text or not isinstance(self.input_text, (str, list)):
Review Comment:
This is typical pattern for airflow to move templated_fields validation to execute.
The parameter will come as string when it templated unless you use `render_template_as_native_obj` in which case it wil getl automatically converted to the object corresponding to what it will look like (i.e. if it looks like array - it will be converted to array (it uses https://jinja.palletsprojects.com/en/3.0.x/nativetypes/) - but then execute will get the converted object already.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org