You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "sunank200 (via GitHub)" <gi...@apache.org> on 2024/05/02 10:00:48 UTC
[PR] Fix the pinecone system test [airflow]
sunank200 opened a new pull request, #39365:
URL: https://github.com/apache/airflow/pull/39365
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Pinecone system test are failing as specs is missing while creating the index in Pinecone. Also the default metric while creating index should be `cosine`.
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587600098
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
Given your point about using an operator instead of the hook method, could you also verify if the hook requires any adjustments? If `hook.create_index(index_name=index_name, dimension=1536)` is no longer functioning, we should investigate what might be causing the issue, as that could be the actual solution. Is it due to missing arguments? In that case, those missing arguments should be incorporated into the hook. They could either be positional arguments, and we could illustrate here in the example the possible values those positional arguments could take. Alternatively, we could designate them as keyword arguments and provide default values that are acceptable.
While altering the example DAG might resolve the issue temporarily, it suggests that there could be other potential issues users might encounter if they fail to provide the required arguments to either the hook method or the operator. I believe the interface itself requires refinement rather than just adjusting the example DAG.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590759423
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
Not very clear yet. But happy with the default to cosine
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590734592
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
Same question here. If `None` is an acceptable value, then why are we defaulting to "cosine"?
--
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] Fix the pinecone system test [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587688144
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
@utkarsharma2 , I did try to do that before coding it up: [issue](https://github.com/apache/airflow/issues/37158#issuecomment-1945287340).
But, i think we still can update this. Since this is yet to be released. Or Am i wrong?
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590648932
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -44,8 +43,8 @@ def create_index():
from airflow.providers.pinecone.hooks.pinecone import PineconeHook
hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
Review Comment:
Do we need this sleep? Maybe it was added in this task to allow some time to setup the index in backend?
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587971097
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
Since it's a major release 2.0.0, having breaking changes should be fine. We should ensure that when initialising hooks and operators if only required params are given by users in their DAG, the initialisation & execution runs successfully i.e. we should have a clear separation of required positional and keywords args, and ensure keyword args have default values if needed for the hook and operator interfaces that are needed when the user has not given those in their DAG.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587580113
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
what did we change exactly?
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587441440
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
Previously the create_index did not require the specs for `pinecone<3`. This was the change done in https://github.com/apache/airflow/pull/37307. I have now used the operator to create index 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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587971097
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
Since it's a major release 2.0.0, having breaking changes should be fine. We should ensure that when initialising hooks and operators if only required params are given by users in their DAG, the initialisation & execution runs successfully i.e. we should have a clear separation of required and keywords args, and ensure keyword args have default values if needed for the hook and operator interfaces that are needed when the user has not given those in their DAG.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587388150
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
are all of these required specs for creation of indexes? If they are not specified do we get failures? If so, we should have default values for them in the operator/hook initialisation.
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
```suggestion
metric: str = "cosine",
```
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590646930
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -116,10 +116,10 @@ def __init__(
replicas: int | None = None,
shards: int | None = None,
pods: int | None = None,
- pod_type: str | None = None,
+ pod_type: str | None = "p1.x1",
Review Comment:
```suggestion
pod_type: str = "p1.x1",
```
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -116,10 +116,10 @@ def __init__(
replicas: int | None = None,
shards: int | None = None,
pods: int | None = None,
- pod_type: str | None = None,
+ pod_type: str | None = "p1.x1",
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
```suggestion
metric: str = "cosine",
```
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587686655
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
It's a breaking change as mentioned in this https://github.com/apache/airflow/pull/37307. This is missing specs as per `hook. def create_index(
self,
index_name: str,
dimension: int,
spec: ServerlessSpec | PodSpec,
metric: str | None = None,
timeout: int | None = None,
)` arguments have changed 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] Fix the pinecone system test [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587688144
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
I did try to do that before coding it up: [issue](https://github.com/apache/airflow/issues/37158#issuecomment-1945287340).
But, i think we still can update this. Since this is yet to be released. Or Am i wrong?
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587408230
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
changed it and this is not required
--
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] Fix the pinecone system test [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587638109
##########
airflow/providers/pinecone/hooks/pinecone.py:
##########
@@ -216,7 +216,7 @@ def create_index(
index_name: str,
dimension: int,
spec: ServerlessSpec | PodSpec,
- metric: str | None = "cosine",
+ metric: str | None = None,
Review Comment:
Could be breaking change if the default value in function `self.pinecone_client.create_index` is different from `cosine`.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590750505
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
The upstream library aspects default to `cosine` as we had discussed on the call. Hence just setting it to default as discussed
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587600098
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
Given your point about using an operator instead of the hook method, could you also verify if the hook requires any adjustments? If hook.create_index(index_name=index_name, dimension=1536) is no longer functioning, we should investigate what might be causing the issue, as that could be the actual solution. Is it due to missing arguments? In that case, those missing arguments should be incorporated into the hook. They could either be positional arguments, and we could illustrate here in the example the possible values those positional arguments could take. Alternatively, we could designate them as keyword arguments and provide default values that are acceptable.
While altering the example DAG might resolve the issue temporarily, it suggests that there could be other potential issues users might encounter if they fail to provide the required arguments to either the hook method or the operator. I believe the interface itself requires refinement rather than just adjusting the example DAG.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587600098
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
also since you're pointing that we need to use an operator instead of the hook method, could you also please check if the hook needs some fixing?
If `hook.create_index(index_name=index_name, dimension=1536)` is no longer working, we should check what is it that is breaking and that could be the real fix. Is it missing args? Then those missing args should be added to the hook, they could be positional args and we could show here in the example what values those positional args could take. Or we could mark them as keyword args and provide default values that are accepted.
--
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] Fix the pinecone system test [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587688144
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
@utkarsharma2 , ~~I did try to do that before coding it up: [issue](https://github.com/apache/airflow/issues/37158#issuecomment-1945287340).~~ i misunderstood.
But, i think we still can update this. Since this is yet to be released. Or Am i wrong?
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590600760
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
I checked it from the upstream library. It defaults to `cosine`. Hence keeping it back to `cosine`.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590723648
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -44,8 +43,8 @@ def create_index():
from airflow.providers.pinecone.hooks.pinecone import PineconeHook
hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
Review Comment:
Then should we add this sleep back below once after calling create_index because we're removing it and it may cause probs intermittently for the DAG?
--
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] Fix the pinecone system test [airflow]
Posted by "phanikumv (via GitHub)" <gi...@apache.org>.
phanikumv merged PR #39365:
URL: https://github.com/apache/airflow/pull/39365
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587405442
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
No, I don't think it's required. But this task was added to the new pinecone https://github.com/apache/airflow/pull/37307. I have just reused it and tested it
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590601056
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
Added a example DAG with bare minimum arguments
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590632093
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
I have changed the example DAG as discussed @pankajkoti
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590655107
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
So was the conclusion that we need to set a default value `pod_type: str = "p1.x1"`? Or is `pod_type = None` allowed too?
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587580113
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
what did we change exactly? I see we're changing the default value to a string "cosine". Since we're not longer keeping None as default value here, my suggestion was to remove `| None` from the param 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] Fix the pinecone system test [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587740305
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
I think the release is ongoing - https://pypi.org/project/apache-airflow-providers-pinecone/2.0.0rc1/. But you can add -1 vote for the pinecone provider in the mailing list if you are willing to make the change and then the next iteration or rc2 can have all the changes. I would personally do it. Since exposing an interface and making changes later we need to maintain the backward compatibility.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587434689
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
`specs` arguments are not compulsory. All the arguments in the `specs` are optional.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590740369
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
As discussed, the upstream library allows None and sets `p1.x1` to default. This is weird in python terms. Hence setting it to just default value and removing `None`
--
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] Fix the pinecone system test [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1589839098
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
@utkarsharma2 , There was an email in the dev list that pinecone will be excluded from the current 2.0.0rc1 release. I guess we can go ahead to rewrite the provider.
@sunank200 , would you like to take up this? If not, I can work on this. Although, I would request your help in testing pod based as they are not available for free accounts.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590600460
##########
airflow/providers/pinecone/hooks/pinecone.py:
##########
@@ -216,7 +216,7 @@ def create_index(
index_name: str,
dimension: int,
spec: ServerlessSpec | PodSpec,
- metric: str | None = "cosine",
+ metric: str | None = None,
Review Comment:
I checked it from the upstream library. It defaults to `cosine`.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587432202
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
As I read the PR description I see that the system tests are failing due to missing spec. What all are the missing specs that are required? If there are some specs that are required, we should either mark those as required positional args or if they are keyword args they should have default values during the operator initialisation.
https://github.com/apache/airflow/blob/778e8c50b987176b15689bb681ac4c48d7a7805a/airflow/providers/pinecone/operators/pinecone.py#L109
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587580113
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
what did we change exactly? I see we're changing the default value to a string `cosine`. Since we're no longer keeping `None` as default value here, my suggestion was to remove `| None` from the param 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] Fix the pinecone system test [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587618803
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
Trying to answer about why the hook no longer works without adding spec object:
@pankajastro , we have now introduced a required argument which needs to be either a `ServerlessSpec` or a `PodSpec` object. Earlier, this is not the case. In the [migration guide](https://canyon-quilt-082.notion.site/Pinecone-Python-SDK-v3-0-0-Migration-Guide-056d3897d7634bf7be399676a4757c7b) , Please check the section with heading "Creating a pod index". I think this should give an example.
--
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] Fix the pinecone system test [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587688144
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
I did try to do that before coding it up: [issue](https://github.com/apache/airflow/issues/37158#issuecomment-1945287340).
But, i think we still can. Since this is yet to be released. Or Am i wrong?
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587441440
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
Previously the create_index did not require the specs for `pinecone<3`.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587574277
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
Would the below bare minimum operator usage work?
```
create_index = CreatePodIndexOperator(
task_id="create_index",
index_name=index_name,
dimension=1536
)
```
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587408230
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
changed it
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590724948
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
It makes it unclear/abstract on if None is allowed, then why are setting p1.x1 as default and not None.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590741342
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -44,8 +43,8 @@ def create_index():
from airflow.providers.pinecone.hooks.pinecone import PineconeHook
hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
Review Comment:
Added the sleep time as before
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590655107
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
So was the conclusion that we need to set a default value `pod_type: str = "p1.x1"`?
--
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] Fix the pinecone system test [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587618803
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
Trying to answer about why the hook's create_index no longer works without adding spec object:
@pankajastro , we have now introduced a required argument which needs to be either a `ServerlessSpec` or a `PodSpec` object. Earlier, this is not the case. In the [migration guide](https://canyon-quilt-082.notion.site/Pinecone-Python-SDK-v3-0-0-Migration-Guide-056d3897d7634bf7be399676a4757c7b) , Please check the section with heading "Creating a pod index". I think this should give an example.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590740369
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
As discussed, the upstream library allows None and sets `p1.x1` to default. This is weird in Python terms. Hence, we are setting it to the default value and removing `None`.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590704500
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
`pod_type` is set as `p1.x1` by the upstream library as default. `None` is also allowed.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590704995
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -44,8 +43,8 @@ def create_index():
from airflow.providers.pinecone.hooks.pinecone import PineconeHook
hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
Review Comment:
Yes, it was just to allow some time to setup the index in backend.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1590634368
##########
airflow/providers/pinecone/operators/pinecone.py:
##########
@@ -119,7 +119,7 @@ def __init__(
pod_type: str | None = None,
metadata_config: dict | None = None,
source_collection: str | None = None,
- metric: str | None = None,
+ metric: str | None = "cosine",
Review Comment:
Also metric value can be `None` as well
--
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] Fix the pinecone system test [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587601385
##########
airflow/providers/pinecone/hooks/pinecone.py:
##########
@@ -216,7 +216,7 @@ def create_index(
index_name: str,
dimension: int,
spec: ServerlessSpec | PodSpec,
- metric: str | None = "cosine",
+ metric: str | None = None,
Review Comment:
changing the default value from `cosine` to `None` looks weird. can we keep it to "cosine"?
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587661874
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
@rawwar I guess you meant to tag me, my github ID is @pankajkoti :)
Yes, that was exactly my question. We should demonstrate here then how to pass the needed spec argument for the hook's create_index method.
Additionally, we should also check that the operator is able to pass the spec argument as is required to the hook method in its execution. For that we will need to recheck the arguments for the operator initialisation and ensure the signature and interfaces are working.
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587661874
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
@rawwar I guess you meant to tag me, my github ID is @pankajkoti :)
Yes, that was exactly my question. We should demonstrate here then how to pass the needed spec argument.
Additionally, we should also check that the operator is able to pass the spec argument as is required to the hook method in its execution. For that we will need to recheck the arguments for the operator initialisation and ensure the signature and interfaces are working.
--
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] Fix the pinecone system test [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587678297
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
@rawwar I think we should just expose the `PodSpec` to be created by user or we can also use `pod_spec_kwargs: dict` in `CreatePodIndexOperator` and pass it to `PodSpec` object. It saves us the maintenance overhead. Since for example if there is a parameter deprecation or addition to `PodSpec` we need to keep up with the interface.
But I'm afraid we might be little late to make change in the interface now. Since this should have been done in initial [PR](https://github.com/apache/airflow/pull/37307) or next major release.
--
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] Fix the pinecone system test [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587441440
##########
tests/system/providers/pinecone/example_pinecone_cohere.py:
##########
@@ -37,15 +36,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=768)
- time.sleep(60)
+ create_index = CreatePodIndexOperator(
+ task_id="create_index",
+ index_name=index_name,
+ dimension=768,
+ replicas=1,
+ shards=1,
+ pods=1,
+ pod_type="p1.x1",
Review Comment:
Previously the create_index did not require the specs for `pinecone<3`. This was the change done in https://github.com/apache/airflow/pull/37307
--
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] Fix the pinecone system test [airflow]
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #39365:
URL: https://github.com/apache/airflow/pull/39365#discussion_r1587974655
##########
tests/system/providers/pinecone/example_pinecone_openai.py:
##########
@@ -75,15 +74,15 @@
start_date=datetime(2023, 1, 1),
catchup=False,
) as dag:
-
- @setup
- @task
- def create_index():
- from airflow.providers.pinecone.hooks.pinecone import PineconeHook
-
- hook = PineconeHook()
- hook.create_index(index_name=index_name, dimension=1536)
Review Comment:
My opinion is that we should show how to create an index using the create_index hook method. Additionally, we should make sure that the operator is able to create index by providing required set of positional arguments and that keyword arguments have default values specified so that if the user does not provide those values the index is created seamlessly from the operator.
--
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