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/12/24 19:05:07 UTC
[PR] Add support for openlineage to AFS and common.io [airflow]
bolkedebruin opened a new pull request, #36410:
URL: https://github.com/apache/airflow/pull/36410
This adds low level support for open lineage to ObjectStorage and integrates it into common.io.
<!--
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] Add support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436939084
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
I am not sure - what's the semantic of key vs. path. What's the "semantic" difference between those I wonder?
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436422586
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
I think we should have some flexibility here. There is at least local filesystem where semantic is different depending on presence or lack of the leading slash. Specifically local filesystem makes a difference:
```
file:///tmp/test.json
```
is different tham:
```
file://tmp/test.json
```
The latter is a file in temporary subfolder in current working directory, where the former is "/tmp/test.json" starting from the root. I think stripping behaviour should depend on the filesystem.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #36410:
URL: https://github.com/apache/airflow/pull/36410#issuecomment-1877054654
ptal @hussein-awala @potiuk @uranusjr. The doc caching update has been split from this pr, I've kept the resolution of a `key` relative per examples and best practices. Integrating 'emitting' open lineage events requires work on the openlineage side per @mobuchowski.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436921924
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Oh yes, lstrip removes all slashes while only 1 needs to be removed. I do prefer not to distinguish based of filesystem, as we call our implementation ObjectStoragePath and document it to have object storage semantics.
But indeed here I think it needs to be
```python
if s.startswith('/'):
s = s[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] Add support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436954455
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Furthermore given https://en.wikipedia.org/wiki/File_URI_scheme a format of "file://tmp/file" is actually not valid when omitting the host, ie. it always needs to be an absolute reference. So your example is invalid.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436422586
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
I think we should have some flexibility here. There is at least local filesystem where semantics is different depending on presence or lack of the leading slash. Specifically for local filesystem it makes a difference:
```
file:///tmp/test.json
```
is different tham:
```
file://tmp/test.json
```
The latter is a file in a `tmp` subfolder in current working directory, where the former is "/tmp/test.json" starting from the root. I think stripping behaviour should depend on the filesystem.
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
I think we should have some flexibility here. There is at least local filesystem where semantics is different depending on presence or lack of the leading slash. Specifically for local filesystem it makes a difference:
```
file:///tmp/test.json
```
is different tham:
```
file://tmp/test.json
```
The latter is a file in the `tmp` subfolder in current working directory, where the former is "/tmp/test.json" starting from the root. I think stripping behaviour should depend on the filesystem.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "mobuchowski (via GitHub)" <gi...@apache.org>.
mobuchowski commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1437100635
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Local storage is inherently unreliable when talking about lineage :)
It would definitely be the best to have absolute references though.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "mobuchowski (via GitHub)" <gi...@apache.org>.
mobuchowski commented on PR #36410:
URL: https://github.com/apache/airflow/pull/36410#issuecomment-1870396524
@bolkedebruin working on it 🙂 https://github.com/apache/airflow/commit/f15a1e01011f1a51fec82d5fcfefa44ba3bc1778
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #36410:
URL: https://github.com/apache/airflow/pull/36410#issuecomment-1868580098
should I just add openlineage to the list to 'always' test @potiuk ?
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436927983
##########
docs/apache-airflow/core-concepts/objectstorage.rst:
##########
@@ -305,6 +311,27 @@ Airflow needs to walk the directory tree and copy each file individually. This i
source to the target.
+Caching
Review Comment:
I think it's fine to keep several unrelated changes in "suppplementary" code - like dev tools etc - we are always cherry-picking pretty much "all" changes there. However in the "core" code it's very important to keep unrelated changes - even documentation ones - separate - because it makes such changes easier cherry-pickable and makes release notes cleaner and easier to follow (those two changes should be mentioned in different sections of the relase notes).
Simply speaking - by having unrelated changes in one PR you make your life slightly easier while release manager life quite a lot more complex.
So yes, I think it should be split.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436018519
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Lstrip a path doesn’t sound right to me; I’d instead prefer ensuring a slash is present in the front. (Although tachnically the two probably are equivalent since this is just a key and not actually semantically need to be a valid path…)
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436064768
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
It depends how we want the key to an object to look look like
What will this do:
```
s3://my-bucket/my/key
bucket = my-bucket # notice no '/'
key = "my/key"
# or
key = "/my/key"
```
The S3 hook (https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/_modules/airflow/providers/amazon/aws/hooks/s3.html#S3Hook.get_s3_bucket_key) is explicit about making it a relative path (so without the initial '/') and thus generates the first version.
I agree with adding a rstrip or then basically a full `strip`.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436922470
##########
docs/apache-airflow/core-concepts/objectstorage.rst:
##########
@@ -305,6 +311,27 @@ Airflow needs to walk the directory tree and copy each file individually. This i
source to the target.
+Caching
Review Comment:
It is indeed unrelated and I am piggy backing here :-). Objections to keeping it? Otherwise I will separate 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 support for openlineage to AFS and common.io [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1435889890
##########
airflow/io/path.py:
##########
@@ -52,6 +52,9 @@ def __init__(
conn_id: str | None = None,
**kwargs: typing.Any,
) -> None:
+ # warning: we are not calling super().__init__ here
+ # as it will try to create a new fs from a different
+ # set if registered filesystems
Review Comment:
đź‘Ť
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Is this a bug fix? If so, moving it to a separate PR would be better, because it looks like a breaking change in this PR.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436939872
##########
docs/apache-airflow/core-concepts/objectstorage.rst:
##########
@@ -305,6 +311,27 @@ Airflow needs to walk the directory tree and copy each file individually. This i
source to the target.
+Caching
Review Comment:
Okido. I'll split 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 support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436422586
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
I think we should have some flexibility here. There is at least local filesystem where semantics is different depending on presence or lack of the leading slash. Specifically for local filesystem it makes a difference:
```
file:///tmp/test.json
```
is different tham:
```
file://tmp/test.json
```
The latter is a file in temporary subfolder in current working directory, where the former is "/tmp/test.json" starting from the root. I think stripping behaviour should depend on the filesystem.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #36410:
URL: https://github.com/apache/airflow/pull/36410#issuecomment-1870198238
@mobuchowski if there is any way to *emit* lineage events and openlineage can reconcile those we can add those events at the lower level so lineage becomes available for TaskFlow as well without manual intervention. Not sure if we can though?
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1435997508
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Somewhat, but there is nothing that relies on it. I need it to be equal to what S3 provides and to deliver equal results. I need it in this PR though, otherwise I need to add an additional 'bugfix'. I dont think that makes sense.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436951129
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
disclaimer: i'm not sure what is right here either.
Given that documentation on object stores like [[1]](https://docs.openstack.org/api-ref/object-store/#create-or-replace-object) [[2]](https://www.educative.io/answers/what-do-keys-mean-in-amazon-s3) [[3]](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html) [[4]](https://learn.microsoft.com/en-us/azure/storage/blobs/storage-blobs-list) all use *relative* identifiers to objects/keys and they construct an URI to to an object as follows:
```python
uri = f"{protocol}://{container}/{object}" # note the hardcoded '/'
# or in s3 speak
uri = f"{protocol}://{bucket}/{key}" # again the hardcoded '/'
```
Double slashes don't make sense here because if referencing a local file the bucket would be "" (null/empty):
```python
b = ObjectStoragePath("file:///tmp/data.json")
assert b.bucket == ""
```
Reconstructing the uri would then still work:
```python
uri = f"{b.protocol}://{b.bucket}/{b.path}" # uri = "file:///tmp/data.json"
```
I think the proper way to reference a key/object is relative and thus stripping a slash.
`path` is obtained from the urlsplit and we always assume a non-relative url that is passed to ObjectStoragePath. So considering that I think stripping the leading slashes from a `key` is the correct behavior.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436954455
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Furthermore given https://en.wikipedia.org/wiki/File_URI_scheme a format of "file://tmp/file" is actually not valid when omitting the host, ie. it always needs to be an absolute reference.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436064768
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
It depends how we want the key to an object to look look like
What will this do:
```
s3://my-bucket/my/key
bucket = my-bucket # notice no '/'
key = "my/key"
# or
key = "/my/key"
```
The S3 hook (https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/_modules/airflow/providers/amazon/aws/hooks/s3.html#S3Hook.get_s3_bucket_key) is explicit about making it a relative path (so with the initial '/').
I agree with adding a rstrip or then basically a full `strip`.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436030085
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
On the other hand, should we do a rstrip? That’s what Python’s pathlib does, and while a trailing slash is technically significant in a URI, removing it seems to make sense here since only one of the variants can make sense for any particular existing resource.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436921924
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
Oh yes, lstrip removes all slashes while only 1 needs to be removed. I do prefer not to distinguish based of filesystem, as we call our implementation ObjectStoragePath and document it to have object storage semantics.
But indeed here I think it needs to be
```python
if s.startswith('/'):
s = s[1:]
```
edit: maybe not: `objectstorage.path` gives you the real path, the key is without the slash:
```python
b = ObjectStoragePath("file:///tmp/test.json")
b.bucket # empty
b.path # /tmp/test.json
b.key # tmp/test.json
```
The above seems to me to be the correct semantics. 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] Add support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436422586
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
I think we should have some flexibility here. There is at least local filesystem where semantic is different depending on presence or lack of the leading slash. Specifically for local filesystem it makes a difference:
```
file:///tmp/test.json
```
is different tham:
```
file://tmp/test.json
```
The latter is a file in temporary subfolder in current working directory, where the former is "/tmp/test.json" starting from the root. I think stripping behaviour should depend on the filesystem.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin merged PR #36410:
URL: https://github.com/apache/airflow/pull/36410
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436422586
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
I thin we should have some flexibility here. There is at least local filesystem where semantic is different depending on presence or lack of the leading slash. Specifically local filesystem makes a difference:
```
file:///tmp/test.json
```
is different tham:
```
file://tmp/test.json
```
The latter is a file in temporary subfolder in current working directory, where the former is "/tmp/test.json" starting from the root. I think stripping behaviour should depend on the filesystem.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436064768
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
It depends how we want the key to an object to look look like
What will this do:
```
s3://my-bucket/my/key
bucket = my-bucket # notice no '/'
key = "my/key"
# or
key = "/my/key"
```
The S3 hook (https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/_modules/airflow/providers/amazon/aws/hooks/s3.html#S3Hook.get_s3_bucket_key) is explicit about making it a relative path (so without the initial '/') and thus generates the first version.
I agree with adding a rstrip or then basically a full `strip`. <--- strike that. `mkdir` and `_cp_file` will check for a trailing slash to have directory like semantics.
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #36410:
URL: https://github.com/apache/airflow/pull/36410#issuecomment-1868760665
cc @mobuchowski
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436951129
##########
airflow/io/path.py:
##########
@@ -173,10 +176,14 @@ def bucket(self) -> str:
@property
def key(self) -> str:
if self._url:
- return self._url.path
+ return self._url.path.lstrip(self.sep)
Review Comment:
disclaimer: i'm not sure what is right here either.
Given that documentation on object stores like [[1]](https://docs.openstack.org/api-ref/object-store/#create-or-replace-object) [[2]](https://www.educative.io/answers/what-do-keys-mean-in-amazon-s3) [[3]](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html) [[4]](https://learn.microsoft.com/en-us/azure/storage/blobs/storage-blobs-list) all use *relative* identifiers to objects/keys and they construct an URI to to an object as follows:
```python
uri = f"{protocol}://{container}/{object}" # note the hardcoded '/'
# or in s3 speak
uri = f"{protocol}://{bucket}/{key}" # again the hardcoded '/'
```
Double slashes don't make sense here because if referencing a local file the bucket would be "" (null/empty):
```python
b = ObjectStoragePath("file:///tmp/data.json")
assert b.bucket == ""
```
Reconstructing the uri would then still work:
```python
uri = f"{b.protocol}://{b.bucket}/{b.path}" # uri = "file:///tmp/data.json"
```
I think the proper way to reference a key/object is relative and thus stripping a slash.
`path` is obtained from the urlsplit and we always assume a non-relative url that is passed to ObjectStoragePath. So considering that I think stripping the leading slashes from a `key` is the correct behavior.
Note: The S3 Hook also strips the leading slash for keys
--
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 support for openlineage to AFS and common.io [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36410:
URL: https://github.com/apache/airflow/pull/36410#discussion_r1436420411
##########
docs/apache-airflow/core-concepts/objectstorage.rst:
##########
@@ -305,6 +311,27 @@ Airflow needs to walk the directory tree and copy each file individually. This i
source to the target.
+Caching
Review Comment:
Should we separate it out ? I think that's unrelated to open lineage?
--
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