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

ENH: add support to read, write, list and remove /vsimem/ files #457

Merged

Conversation

theroggy
Copy link
Member

@theroggy theroggy commented Aug 3, 2024

Add support for the basic operations on /vsimem/ files: read, write, list and remove.

TODO:

Triggered by a topic in the "discussion" section of geopandas, where someone noted that pre-geopandas 1.0 this worked and now it does not anymore: geopandas/geopandas#3376

<-- clarified below
I now noticed the following statement in: #407

This also adds a specific check that the incoming path does not already contain /vsimem/, since we have to handle that internally and cannot allow it to be passed in.

@brendan-ward I wonder... is this meant as:

  • an explicit choice not ever to want to allow /vsimem/ inputs because ???
  • or a temporary measure till support has been built in
  • or is there some technical reason that this causes problems that I didn't think about/haven't encountered yet

-->

@brendan-ward
Copy link
Member

a temporary measure till support has been built in

Most likely this, though probably wasn't going to prioritize fixing it unless there was a need (apparently there is).

@theroggy theroggy self-assigned this Aug 5, 2024
@theroggy theroggy changed the title ENH: add support to use /vsimem/ input files ENH: add support to to read/write from/to /vsimem/ files Aug 9, 2024
@theroggy theroggy changed the title ENH: add support to to read/write from/to /vsimem/ files ENH: Add support for all basic operations on /vsimem/ files Aug 9, 2024
@theroggy theroggy changed the title ENH: Add support for all basic operations on /vsimem/ files ENH: Add support for the basic operations on /vsimem/ files Aug 9, 2024
@theroggy theroggy changed the title ENH: Add support for the basic operations on /vsimem/ files ENH: Add support for the basic file operations on /vsimem/ files Aug 9, 2024
@theroggy theroggy changed the title ENH: Add support for the basic file operations on /vsimem/ files ENH: add support to read, write, list and remove /vsimem/ files Aug 9, 2024
@theroggy theroggy marked this pull request as ready for review August 10, 2024 15:35
Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @theroggy and apologies for the slow review! I only had a chance to review up to your tests and will try to review those in the next couple of days.

CHANGES.md Outdated Show resolved Hide resolved
pyogrio/_io.pyx Outdated Show resolved Hide resolved
pyogrio/_io.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/core.py Show resolved Hide resolved
pyogrio/core.py Outdated Show resolved Hide resolved
pyogrio/tests/test_core.py Outdated Show resolved Hide resolved
Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

A few more comments, thanks for the updates so far @theroggy , and thanks for your patience with the review.

pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Show resolved Hide resolved
pyogrio/core.py Show resolved Hide resolved
pyogrio/tests/test_arrow.py Outdated Show resolved Hide resolved
pyogrio/tests/test_core.py Show resolved Hide resolved
pyogrio/tests/test_core.py Outdated Show resolved Hide resolved
pyogrio/tests/test_core.py Show resolved Hide resolved
pyogrio/tests/test_path.py Show resolved Hide resolved
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Great work!

.github/workflows/lint.yml Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
pyogrio/_io.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/_vsi.pyx Outdated Show resolved Hide resolved
pyogrio/tests/test_core.py Outdated Show resolved Hide resolved
pyogrio/tests/test_core.py Outdated Show resolved Hide resolved
pyogrio/tests/test_core.py Show resolved Hide resolved
pyogrio/tests/test_geopandas_io.py Outdated Show resolved Hide resolved
pyogrio/tests/test_path.py Show resolved Hide resolved
@brendan-ward brendan-ward added this to the 0.10.0 milestone Sep 16, 2024
@brendan-ward
Copy link
Member

I think it can be done in a later PR (but preferably for 0.10.0), but we should probably document how the /vsimem interface can be used more directly (e.g., alongside gdal package) as well as the VSI functions that we want to expose publicly here (which is why I think it is should be follow-up, so that this PR focuses on the API).

Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks @theroggy ! @jorisvandenbossche any final comments or disagreement with the plan to add the vsi_* functions at the top-level for now?

@brendan-ward brendan-ward merged commit 52c33f3 into geopandas:main Sep 27, 2024
21 checks passed
@theroggy theroggy deleted the ENH-make-it-possible-to-use-/vsimem- branch September 27, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants