Fix ObjectStoragePath compatible with universal-pathlib 0.3.0 to avoid maximum recursion depth exceeded #56270

gopidesupavan · 2025-09-30T21:10:00Z

universal-pathlib version 0.3.0 introduced several changes. One notable update is that the path property now uses str(self) internally https://github.com/fsspec/universal_pathlib/blob/v0.3.0/upath/core.py#L218-L234. This leads to a recursion error in our case because ObjectStoragePath implements a custom str method, and accessing self.path triggers an infinite loop. To resolve this, we should switch to using _raw_urlpaths instead.

https://pypi.org/project/universal-pathlib/

https://github.com/apache/airflow/actions/runs/18141256490/job/51633607047

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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 airflow-core/newsfragments.

…d maximum recursion depth exceeded

jscheffl

FYI @bolkedebruin

eladkal · 2025-09-30T21:17:24Z

task-sdk/src/airflow/sdk/io/path.py

            shutil.copyfileobj(f1, f2, **kwargs)

-    def copy(self, dst: str | ObjectStoragePath, recursive: bool = False, **kwargs) -> None:
+    def copy(self, dst: str | ObjectStoragePath, recursive: bool = False, **kwargs) -> None:  # type: ignore[override]


we normally don't use ignores unless absolutely necessary as ignores don't really fix stuff but just hide the issue.
isn't there a way to fix without ignore?

tbh i dont have any idea the super class signature is like this:

task-sdk/src/airflow/sdk/io/path.py:297: note: Superclass: task-sdk/src/airflow/sdk/io/path.py:297: note: def [T: WritablePath] copy(self, target: T, **kwargs: Any) -> T task-sdk/src/airflow/sdk/io/path.py:297: note: Subclass: task-sdk/src/airflow/sdk/io/path.py:297: note: def copy(self, dst: str | ObjectStoragePath, recursive: bool = ..., **kwargs: Any) -> None

calling for the expert @uranusjr

okay i removed that now.

didnt worked mypy still not happy

bolkedebruin · 2025-10-01T06:26:45Z

I think it looks okay. assuming is_relative_to an api update. If that is the case, it could warrant a release notes update event though it is downstream update?

gopidesupavan · 2025-10-01T07:22:16Z

I think it looks okay. assuming is_relative_to an api update. If that is the case, it could warrant a release notes update event though it is downstream update?

yeah it looks like it has updates https://github.com/fsspec/universal_pathlib/pull/405/files

gopidesupavan · 2025-10-01T07:23:53Z

will be away for whole day please feel free to merge this, mypy still not happy and have added ignore for now.

bolkedebruin · 2025-10-01T08:51:51Z

I think it looks okay. assuming is_relative_to an api update. If that is the case, it could warrant a release notes update event though it is downstream update?

yeah it looks like it has updates https://github.com/fsspec/universal_pathlib/pull/405/files

I couldn't find it in that pull request. https://github.com/fsspec/universal_pathlib/blob/main/upath/core.py#L1074 still has relative_to which behaves differently than is_relative_to. So to me this seems an unrelated change?

bolkedebruin

Let's remove the unrelated change from the tests or explain why it is required.

task-sdk/src/airflow/sdk/io/path.py

gopidesupavan · 2025-10-03T06:46:12Z

Let's remove the unrelated change from the tests or explain why it is required.

sorry what i meant is , the relative_to function implementation changed.

in 0.3.0 its returning

in 0.2.6

earlier versions the implementation of relative_to function is in the cloudpath: https://github.com/fsspec/universal_pathlib/pull/405/files#diff-311a53e84d3cff9dd608444fabb9f22eace31a4f35cdd481083d1761296e41cdL63 but its removed from the cloud path instead it is now implemented in core with slight modification that returns _relative_base https://github.com/fsspec/universal_pathlib/blob/main/upath/core.py#L1074

So the test failing with the current implementation of relative_to:

i could change the assertion from assert o1.relative_to(o2) == o1 -> assert o1.relative_to(o2) hope its fine?

potiuk

For me the explanation is clear and reasonable.

amoghrajesh

Makes sense to me

ashb · 2025-10-03T09:58:28Z

task-sdk/src/airflow/sdk/io/path.py

        conn_id = self.storage_options.get("conn_id")
        if self._protocol and conn_id:
-            return f"{self._protocol}://{conn_id}@{self.path}"
+            return f"{self._protocol}://{conn_id}@{self.parser.join(*self._raw_urlpaths)}"


Is this really the only options? Relying on a private method doesn't feel great.

Could we do this instead:

super(type(self), self).__str__(self.path)

It's much more verbose, and opaque, yes, but it doesn't depend on accessing a "private" method?
Or use self.parts somehow?

As per your comment below, we should likely restrict 0.3.0 and use this library properly

ashb

I don't know if this matters, but us overloading str like this also likely breaks other aspects of this library (either by causing a recursion error elsewhere, or by including other info that the upath library isn't expecting.

I think we should probably report this as a bug upstream and block 0.3.0 for the moment instead.

For example, self.parts is broken now (and I think this is just 0.3.0:

On 0.2.6:

(Pdb++) self.parts
('/', 'bucket', 'path')

vs 0.3.0

(Pdb++) self.parts
('/', 'bucket', 'ath')
(Pdb++) self
ObjectStoragePath('fake@bucket/path', protocol='fake')

Okay yeah, this is very much an "we are using this library really really wrong":

(Pdb++) CloudPath("s3://bucket/path")
S3Path('bucket/path', protocol='s3')
(Pdb++) CloudPath("s3://bucket/path").parts
('bucket/', 'path')
(Pdb++) upath.__version__
'0.3.0'

gopidesupavan · 2025-10-03T17:28:10Z

closing it for now: #56370

gopidesupavan · 2025-10-03T17:28:57Z

created issue here closing it for now: #56370

Fix ObjectStoragePath compatible with universal-pathlib 0.3.0 to avoi…

c85676d

…d maximum recursion depth exceeded

gopidesupavan requested review from amoghrajesh, ashb and kaxil as code owners September 30, 2025 21:10

boring-cyborg bot added the area:task-sdk label Sep 30, 2025

jscheffl approved these changes Sep 30, 2025

View reviewed changes

eladkal reviewed Sep 30, 2025

View reviewed changes

gopidesupavan mentioned this pull request Sep 30, 2025

Add e2e test for remote logging #56191

Merged

gopidesupavan force-pushed the fix-upath-recursion-isse branch from 8f78f58 to a0415dc Compare October 1, 2025 06:14

potiuk approved these changes Oct 1, 2025

View reviewed changes

Attemp mypy fix

d9953f4

gopidesupavan force-pushed the fix-upath-recursion-isse branch from a0415dc to d9953f4 Compare October 1, 2025 07:22

bolkedebruin self-requested a review October 1, 2025 08:52

bolkedebruin requested changes Oct 1, 2025

View reviewed changes

amoghrajesh reviewed Oct 3, 2025

View reviewed changes

task-sdk/src/airflow/sdk/io/path.py Show resolved Hide resolved

task-sdk/src/airflow/sdk/io/path.py Show resolved Hide resolved

Update test_relative_to test assertion

4621d9f

gopidesupavan requested a review from bolkedebruin October 3, 2025 06:57

potiuk approved these changes Oct 3, 2025

View reviewed changes

Add todo comment for type ignore

84b083d

amoghrajesh approved these changes Oct 3, 2025

View reviewed changes

ashb reviewed Oct 3, 2025

View reviewed changes

This was referenced Oct 3, 2025

Rework universal-pathlib in ObjectStoragePath #56369

Open

Restrict universal-pathlib 0.3.0 #56370

Merged

gopidesupavan closed this Oct 3, 2025

Fix ObjectStoragePath compatible with universal-pathlib 0.3.0 to avoid maximum recursion depth exceeded #56270

Fix ObjectStoragePath compatible with universal-pathlib 0.3.0 to avoid maximum recursion depth exceeded #56270

Uh oh!

Conversation

gopidesupavan commented Sep 30, 2025

Uh oh!

jscheffl left a comment

Choose a reason for hiding this comment

Uh oh!

eladkal Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

gopidesupavan Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

eladkal Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

gopidesupavan Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gopidesupavan Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

bolkedebruin commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gopidesupavan commented Oct 1, 2025

Uh oh!

gopidesupavan commented Oct 1, 2025

Uh oh!

bolkedebruin commented Oct 1, 2025

Uh oh!

bolkedebruin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gopidesupavan commented Oct 3, 2025

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

ashb Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

amoghrajesh Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ashb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gopidesupavan commented Oct 3, 2025

Uh oh!

gopidesupavan commented Oct 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gopidesupavan Sep 30, 2025 •

edited

Loading

bolkedebruin commented Oct 1, 2025 •

edited

Loading

ashb left a comment •

edited

Loading