Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: UPath.__new__ handles os.PathLike objects incorrectly #195

Closed
ap-- opened this issue Feb 19, 2024 · 3 comments · Fixed by #200
Closed

Regression: UPath.__new__ handles os.PathLike objects incorrectly #195

ap-- opened this issue Feb 19, 2024 · 3 comments · Fixed by #200
Labels
bug 🐛 Something isn't working

Comments

@ap--
Copy link
Collaborator

ap-- commented Feb 19, 2024

current implementation calls str on an os.Pathlike instance, potentially receiving the wrong urlpath...

class A:
    def __fspath__(self):
        return "s3://bucket/abc"

    def __str__(self):
        return "A('s3://bucket/abc')"
@ap-- ap-- added the bug 🐛 Something isn't working label Feb 19, 2024
@ap--
Copy link
Collaborator Author

ap-- commented Feb 19, 2024

Note: behavior is correct for PurePath subclasses.

@bolkedebruin
Copy link

I'm not sure if it is related. In Airflow using ObjectStoragePath straight out with 0.2.0 will do this:

>>> from airflow.io.path import ObjectStoragePath
/Users/bolke/dev/airflow/airflow/io/path.py:47 DeprecationWarning: All _FSSpecAccessor subclasses have been deprecated.  Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
/Users/bolke/dev/airflow/airflow/io/path.py:72 DeprecationWarning: Detected a customized `__new__` method in subclass 'ObjectStoragePath'. Protocol dispatch will be disabled for this subclass. Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
/Users/bolke/dev/airflow/airflow/io/path.py:72 DeprecationWarning: Detected a customized `__init__` method or `_fs` attribute in the provided `_FSSpecAccessor` subclass of 'ObjectStoragePath'. It is recommended to instead override the `UPath._fs_factory` classmethod to customize filesystem instantiation. Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
>>> p = ObjectStoragePath("file:///tmp/bla")
/Users/bolke/dev/airflow/airflow/io/path.py:154 DeprecationWarning: UPath._from_parts is deprecated and should not be used. Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
>>> str(p)
'file://tmp/bla'  # should be file:///tmp/bla - triple slash

@ap--
Copy link
Collaborator Author

ap-- commented Feb 19, 2024

I'm not sure if it is related. In Airflow using ObjectStoragePath straight out with 0.2.0 will do this:

>>> from airflow.io.path import ObjectStoragePath
/Users/bolke/dev/airflow/airflow/io/path.py:47 DeprecationWarning: All _FSSpecAccessor subclasses have been deprecated.  Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
/Users/bolke/dev/airflow/airflow/io/path.py:72 DeprecationWarning: Detected a customized `__new__` method in subclass 'ObjectStoragePath'. Protocol dispatch will be disabled for this subclass. Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
/Users/bolke/dev/airflow/airflow/io/path.py:72 DeprecationWarning: Detected a customized `__init__` method or `_fs` attribute in the provided `_FSSpecAccessor` subclass of 'ObjectStoragePath'. It is recommended to instead override the `UPath._fs_factory` classmethod to customize filesystem instantiation. Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
>>> p = ObjectStoragePath("file:///tmp/bla")
/Users/bolke/dev/airflow/airflow/io/path.py:154 DeprecationWarning: UPath._from_parts is deprecated and should not be used. Please follow the universal_pathlib==0.2.0 migration guide at https://github.com/fsspec/universal_pathlib for more information.
>>> str(p)
'file://tmp/bla'  # should be file:///tmp/bla - triple slash

This is unrelated. The implementation of ObjectStoragePath on current airflow main is too tightly bound to internals of universal_pathlib==0.1.4

When I refactored the UPath implementation for python 3.12 support I was not able to provide 100% backwards-compatibility. I managed to support a large subset of custom UPath subclasses that I found on github, and even a good chunk of the ObjectStoragePath functionality, but not all of it.

So for universal_pathlib>=0.2.0 support, ObjectStoragePath needs to be refactored.
Cross-referencing: apache/airflow#37524

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants