You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ephraimbuddy (via GitHub)" <gi...@apache.org> on 2023/11/30 16:12:07 UTC
[PR] Add a cache for weaviate client [airflow]
ephraimbuddy opened a new pull request, #35983:
URL: https://github.com/apache/airflow/pull/35983
While working on another issue, I realized how often I had to call get_conn. So instead of depreccating this, we can use it as a cache within the code so we don't connect everytime a method is called.
--
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] Add a cache for weaviate client [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1412024381
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
- # Keeping this for backwards compatibility
- warnings.warn(
- "The `get_client` method has been renamed to `get_conn`",
- AirflowProviderDeprecationWarning,
- stacklevel=2,
- )
+ """Returns a Weaviate client."""
return self.get_conn()
Review Comment:
Instead of removing the deprecation warning from this method, you can make `get_conn` a cached property and change this method to:
```suggestion
return self.get_conn
```
In that way, you will achieve what you want to do without replacing `self.get_client()` with `self.get_client`, and this method will stay deprecated.
--
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] Add a cache for weaviate client [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1411876566
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
Review Comment:
yep, what I meant is something like
```python
@cached_property
def conn(...):
```
--
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] Add a cache for weaviate client [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1411881928
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
Review Comment:
ads: https://github.com/apache/airflow/blob/396b1ba34e7df28eebfafc174494812aa50af966/airflow/providers/google/ads/hooks/ads.py#L162
opensearch:
https://github.com/apache/airflow/blob/396b1ba34e7df28eebfafc174494812aa50af966/airflow/providers/opensearch/hooks/opensearch.py#L56
adx:
https://github.com/apache/airflow/blob/396b1ba34e7df28eebfafc174494812aa50af966/airflow/providers/microsoft/azure/hooks/adx.py#L127
It's just to cache the connection we have made and use 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] Add a cache for weaviate client [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1411923562
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
Review Comment:
> yep, what I meant is something like
>
> ```python
> @cached_property
> def conn(...):
> ```
I think we shouldn't depreccate since there's no convention
--
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] Add a cache for weaviate client [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1411865058
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
Review Comment:
I'm not sure whether the name `client` makes more sense here, but if we're to go with `client`. It means we still need to deprecate this. and `conn` might be an even better name in that case
--
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] Add a cache for weaviate client [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1412024381
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
- # Keeping this for backwards compatibility
- warnings.warn(
- "The `get_client` method has been renamed to `get_conn`",
- AirflowProviderDeprecationWarning,
- stacklevel=2,
- )
+ """Returns a Weaviate client."""
return self.get_conn()
Review Comment:
Instead of removing the deprecation warning from this method, you can make `get_conn` a cached property and change this method to:
```suggestion
return self.get_conn
```
In that way, you will achieve what you want to without replacing `self.get_client()` with `self.get_client`, and this method will stay deprecated.
--
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] Add a cache for weaviate client [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1411874620
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
Review Comment:
We already have a `get_conn`. This is just to serve as proxy for it. I think some other providers do this. I will check and revert
--
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] Add a cache for weaviate client [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy merged PR #35983:
URL: https://github.com/apache/airflow/pull/35983
--
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] Add a cache for weaviate client [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1411878369
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
Review Comment:
https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/hooks/redshift_sql.py#L73-L75
--
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] Add a cache for weaviate client [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #35983:
URL: https://github.com/apache/airflow/pull/35983#discussion_r1412063903
##########
airflow/providers/weaviate/hooks/weaviate.py:
##########
@@ -94,18 +93,14 @@ def get_conn(self) -> WeaviateClient:
url=url, auth_client_secret=auth_client_secret, additional_headers=additional_headers
)
+ @cached_property
def get_client(self) -> WeaviateClient:
- # Keeping this for backwards compatibility
- warnings.warn(
- "The `get_client` method has been renamed to `get_conn`",
- AirflowProviderDeprecationWarning,
- stacklevel=2,
- )
+ """Returns a Weaviate client."""
return self.get_conn()
Review Comment:
I don't think having the cache on `get_conn` is a good one but I agree to keep the deprecation. It would be better to have another method like `_weaviate_client` or `conn` like @Lee-W suggested. It means more options for users and doing `hook.get_conn` would be just a statement that IDEs may not fancy.
--
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