You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "bolkedebruin (via GitHub)" <gi...@apache.org> on 2023/11/23 15:03:22 UTC
[PR] Allow storage options to be passed [airflow]
bolkedebruin opened a new pull request, #35820:
URL: https://github.com/apache/airflow/pull/35820
This allows storage options to be passed on to the underlying implementation. It does require a release of the providers as the signature of `get_fs` has changed.
@kaxil @uranusjr @Taragolis
<!--
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/
-->
<!-- 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.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] Allow storage options to be passed [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin merged PR #35820:
URL: https://github.com/apache/airflow/pull/35820
--
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] Allow storage options to be passed [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #35820:
URL: https://github.com/apache/airflow/pull/35820#discussion_r1403531115
##########
airflow/providers/amazon/aws/fs/s3.py:
##########
@@ -45,7 +45,7 @@ class SignError(Exception):
"""Raises when unable to sign a S3 request."""
-def get_fs(conn_id: str | None) -> AbstractFileSystem:
+def get_fs(conn_id: str | None, storage_options: dict[str, str] | None) -> AbstractFileSystem:
Review Comment:
```suggestion
def get_fs(conn_id: str | None, storage_options: dict[str, str] | None = None) -> AbstractFileSystem:
```
##########
airflow/providers/google/cloud/fs/gcs.py:
##########
@@ -39,7 +39,7 @@
schemes = ["gs", "gcs"]
-def get_fs(conn_id: str | None) -> AbstractFileSystem:
+def get_fs(conn_id: str | None, storage_options: dict[str, str]) -> AbstractFileSystem:
Review Comment:
```suggestion
def get_fs(conn_id: str | None, storage_options: dict[str, str] = None) -> AbstractFileSystem:
```
##########
airflow/providers/microsoft/azure/fs/adls.py:
##########
@@ -27,7 +27,7 @@
schemes = ["abfs", "abfss", "adl"]
-def get_fs(conn_id: str | None) -> AbstractFileSystem:
+def get_fs(conn_id: str | None, storage_options: dict[str, Any] | None) -> AbstractFileSystem:
Review Comment:
```suggestion
def get_fs(conn_id: str | None, storage_options: dict[str, Any] | None = None) -> AbstractFileSystem:
```
--
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] Allow storage options to be passed [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #35820:
URL: https://github.com/apache/airflow/pull/35820#discussion_r1416991356
##########
airflow/io/store/__init__.py:
##########
@@ -134,6 +146,6 @@ def attach(
if store := _STORE_CACHE.get(alias):
return store
- _STORE_CACHE[alias] = store = ObjectStore(protocol=protocol, conn_id=conn_id, fs=fs)
+ _STORE_CACHE[alias] = store = ObjectStore(protocol=protocol, conn_id=conn_id, fs=fs, **kwargs)
Review Comment:
It makes me uncomfortable that storage options are only effective in the first call to `attach`; in later calls they will be silently ignored, even when options in the existing storage object does not match. It would be more explicit (thus less prone to user errors) if storage creation and retrieval should be splitted into two functions instead.
--
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] Allow storage options to be passed [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #35820:
URL: https://github.com/apache/airflow/pull/35820#discussion_r1403643831
##########
airflow/providers/google/cloud/fs/gcs.py:
##########
@@ -39,7 +39,7 @@
schemes = ["gs", "gcs"]
-def get_fs(conn_id: str | None) -> AbstractFileSystem:
+def get_fs(conn_id: str | None, storage_options: dict[str, str]) -> AbstractFileSystem:
Review Comment:
I don't think mypy will pass on this :-)
--
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] Allow storage options to be passed [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #35820:
URL: https://github.com/apache/airflow/pull/35820#issuecomment-1847095296
Merging this - if we want to adjust the typing and split the attach function we can do that in the follow ups.
--
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] Allow storage options to be passed [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #35820:
URL: https://github.com/apache/airflow/pull/35820#discussion_r1420242140
##########
airflow/io/__init__.py:
##########
@@ -32,21 +32,25 @@
if TYPE_CHECKING:
from fsspec import AbstractFileSystem
+ from airflow.io.typedef import Properties
Review Comment:
I like the 'signal' of Properties that it automatically says I can only pass [str, str] instead. To me it feels cognitive easier to see Properties than dict[str, str] which has no semantic meaning and less error prone. But I am not really tied up on it.
And yes maybe we could make it into a TypedDict
what do you say?
--
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] Allow storage options to be passed [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #35820:
URL: https://github.com/apache/airflow/pull/35820#discussion_r1416952712
##########
airflow/io/__init__.py:
##########
@@ -32,21 +32,25 @@
if TYPE_CHECKING:
from fsspec import AbstractFileSystem
+ from airflow.io.typedef import Properties
+
+
log = logging.getLogger(__name__)
-def _file(_: str | None) -> LocalFileSystem:
- return LocalFileSystem()
+def _file(_: str | None, storage_options: Properties) -> LocalFileSystem:
+ options = storage_options or {}
Review Comment:
This seems unnecessary since properties are always a dict
--
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] Allow storage options to be passed [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #35820:
URL: https://github.com/apache/airflow/pull/35820#issuecomment-1837431695
Note: something went wrong with rebasing, fixing 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] Allow storage options to be passed [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #35820:
URL: https://github.com/apache/airflow/pull/35820#discussion_r1416951641
##########
airflow/io/__init__.py:
##########
@@ -32,21 +32,25 @@
if TYPE_CHECKING:
from fsspec import AbstractFileSystem
+ from airflow.io.typedef import Properties
Review Comment:
Why does this need a dedicated type alias? Do we plan to make it stricter with say TypedDict? It feels to me it’s better to use `dict[str, str]` instead; slightly repetitive but cognatively easier for readers.
##########
airflow/io/__init__.py:
##########
@@ -32,21 +32,25 @@
if TYPE_CHECKING:
from fsspec import AbstractFileSystem
+ from airflow.io.typedef import Properties
Review Comment:
Why does this need a dedicated type alias? Do we plan to make it stricter with say TypedDict? It feels to me it’s better to use `dict[str, str]` instead; slightly repetitive but cognatively easier for readers.
--
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] Allow storage options to be passed [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #35820:
URL: https://github.com/apache/airflow/pull/35820#discussion_r1420237930
##########
airflow/io/store/__init__.py:
##########
@@ -134,6 +146,6 @@ def attach(
if store := _STORE_CACHE.get(alias):
return store
- _STORE_CACHE[alias] = store = ObjectStore(protocol=protocol, conn_id=conn_id, fs=fs)
+ _STORE_CACHE[alias] = store = ObjectStore(protocol=protocol, conn_id=conn_id, fs=fs, **kwargs)
Review Comment:
Yeah, you have a point. Do you have a suggestion?
--
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