Skip to content

ARROW-9645: [Python] Deprecate pyarrow.filesystem in favor of pyarrow.fs#8149

Closed
jorisvandenbossche wants to merge 9 commits intoapache:masterfrom
jorisvandenbossche:ARROW-9645-deprecate-legacy-filesystems
Closed

ARROW-9645: [Python] Deprecate pyarrow.filesystem in favor of pyarrow.fs#8149
jorisvandenbossche wants to merge 9 commits intoapache:masterfrom
jorisvandenbossche:ARROW-9645-deprecate-legacy-filesystems

Conversation

@jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Sep 9, 2020 •
edited
Loading

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 9, 2020

https://issues.apache.org/jira/browse/ARROW-9645

@jorisvandenbossche jorisvandenbossche force-pushed the ARROW-9645-deprecate-legacy-filesystems branch from b591d25 to c46ff8c Compare September 17, 2020 12:30
@jorisvandenbossche jorisvandenbossche mentioned this pull request Sep 17, 2020
Merged
@jorisvandenbossche jorisvandenbossche force-pushed the ARROW-9645-deprecate-legacy-filesystems branch from 34c84ee to b274fd8 Compare September 30, 2020 16:14
@jorisvandenbossche jorisvandenbossche marked this pull request as ready for review September 30, 2020 16:15
@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Sep 30, 2020

The main remaining items is pa.hdfs.connect(). It feels a bit annoying to deprecate this again, after we let people migrate from HdfsClient to connect (eg https://stackoverflow.com/questions/47400987/hdfs-connect-vs-hdfsclient-in-pyarrow).

I was thinking to keep pa.hdfs.connect, but letting it return the new filesystem? But since the returned object has a different API, that would also break a lot of code ..

@jorisvandenbossche jorisvandenbossche force-pushed the ARROW-9645-deprecate-legacy-filesystems branch 2 times, most recently from fc5b7fd to 8c57769 Compare September 30, 2020 16:43
@pitrou
Copy link
Member

pitrou commented Sep 30, 2020

I think we should deprecate pa.hdfs.connect. We cannot return something else from it...

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Oct 1, 2020

We could in theory also add a deprecation in each method on the legacy HadoopFileSystem, so that any use outside of passing it to one of our own functions accepting a filesystem gets deprecated, but not the construction of the filesystem object itself (and then we can eventually return a different object, and for now convert it internally).

But, even if we do the effort for that, we are still left with this pyarrow.hdfs namespace which only has the connect() function long term ..

@jorisvandenbossche jorisvandenbossche force-pushed the ARROW-9645-deprecate-legacy-filesystems branch from d45a0fe to dfdc622 Compare October 7, 2020 08:52
@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Oct 7, 2020

@github-actions crossbow submit -g integration

@github-actions
Copy link

github-actions bot commented Oct 7, 2020

Revision: dfdc622

Submitted crossbow builds: ursa-labs/crossbow @ actions-607

Task Status
test-conda-python-3.6-pandas-0.23 Github Actions
test-conda-python-3.7-dask-latest Github Actions
test-conda-python-3.7-hdfs-2.9.2 Github Actions
test-conda-python-3.7-kartothek-latest Github Actions
test-conda-python-3.7-kartothek-master Github Actions
test-conda-python-3.7-pandas-latest Github Actions
test-conda-python-3.7-pandas-master Github Actions
test-conda-python-3.7-spark-branch-3.0 Github Actions
test-conda-python-3.7-turbodbc-latest Github Actions
test-conda-python-3.7-turbodbc-master Github Actions
test-conda-python-3.8-dask-master Github Actions
test-conda-python-3.8-jpype Github Actions
test-conda-python-3.8-pandas-latest Github Actions
test-conda-python-3.8-spark-master Github Actions

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Oct 7, 2020

@pitrou @kszucs more feedback here?

pitrou
pitrou reviewed Oct 7, 2020
View reviewed changes
python/pyarrow/__init__.py Outdated
return _LocalFileSystem
elif name == "HadoopFileSystem":
_warnings.warn(_msg.format("HadoopFileSystem", "HadoopFileSystem"),
DeprecationWarning, stacklevel=2)
Copy link
Member

@pitrou pitrou Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use FutureWarning elsewhere.

pitrou
pitrou reviewed Oct 7, 2020
View reviewed changes
python/pyarrow/__init__.py Outdated

if _sys.version_info >= (3, 7):
def __getattr__(name):
if name == "localfs":
Copy link
Member

@pitrou pitrou Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you want to write a loop and/or helper function to avoid all this copy/pasting?

Copy link
Member Author

@jorisvandenbossche jorisvandenbossche Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the info in a dict, and simplified this

pitrou
pitrou reviewed Oct 7, 2020
View reviewed changes
python/pyarrow/filesystem.py Outdated
warnings.warn(
"pyarrow.filesystem.LocalFileSystem is deprecated as of 2.0.0, "
"please use pyarrow.fs.LocalFileSystem instead",
DeprecationWarning, stacklevel=2)
Copy link
Member

@pitrou pitrou Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a helper in pyarrow.util? (and use FutureWarning?)

Copy link
Member Author

@jorisvandenbossche jorisvandenbossche Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a _deprecate_class in pyarrow.util, but that serves a different purpose (it aliases the old name to the new class, but with warning).

And if it is just a helper for raising the warning, I am not sure that is worth it (compared to using a template message that can be filled in).

Copy link
Member Author

@jorisvandenbossche jorisvandenbossche Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved a template to pyarrow.util. Can also make it a function raising the warning if you prefer

pitrou
pitrou reviewed Oct 7, 2020
View reviewed changes
python/pyarrow/filesystem.py Outdated
return LocalFileSystem._get_instance()

try:
import fsspec
Copy link
Member

@pitrou pitrou Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fsspec seems to take some time to import, we should probably lookup directly in sys.modules instead.

@pitrou pitrou closed this in 25d131b Oct 7, 2020
@jorisvandenbossche jorisvandenbossche deleted the ARROW-9645-deprecate-legacy-filesystems branch October 9, 2020 16:09
@rjzamora rjzamora mentioned this pull request Oct 27, 2020
Closed
@asfimport asfimport mentioned this pull request Oct 7, 2020
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou approved these changes

@kszucs kszucs Awaiting requested review from kszucs

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jorisvandenbossche @pitrou