autotest: Add a VSIFile wrapper class #8222

dbaston · 2023-08-14T11:12:56Z

What does this PR do?

This PR adds a gdal.vsi_open() function that returns a file-like object, allowing virtual files to be used as "regular" Python files in some cases. For example, a zipped CSV can be read using the Python csv module:

with gdal.vsi_open(f'/vsizip/prime_meridian.csv') as f:
    records = [x for x in csv.DictReader(f)]

assert records[2]['INFORMATION_SOURCE'] == "Institut Geographique National (IGN), Paris"

Still need to correctly handle different linebreak/OS combinations, ensure errors are raised when accessing a closed file, handle append mode (or raise error), and probably other things.

What are related issues/pull requests?

Tasklist

coveralls · 2023-08-14T12:07:29Z

coverage: 69.146% (-0.002%) from 69.148%
when pulling 3e747a9 on dbaston:python-vsifile-class
into 0890073 on OSGeo:master.

rouault · 2023-08-15T20:09:38Z

swig/include/python/gdal_python.i

+        self._binary = "b" in mode
+        self._encoding = encoding
+
+        self._fp = VSIFOpenL(self._path, self._mode)


should probably throw an exception is None is returned

done, also switched to VSIFOpenExL to capture the error message

rouault · 2023-08-15T20:11:02Z

swig/include/python/gdal_python.i

+        return self
+
+    def __next__(self):
+        chunk_size = 1024


probably that CPLReadLineL() could be used to avoid this reimplementatoin

rcoup · 2023-08-16T09:29:06Z

If it's helpful, here's an implementation we have been using at Koordinates for a while, with a set of tests: feel free to crib anything that's useful. I've licensed it as MIT. Any questions most welcome.

https://gist.github.com/rcoup/5f4b49149ca26084eed4dccfd1b12895

was written quite a while ago — I would guess it could be simpler today incorporating some stdlib updates in recent Python versions and/or other bugfixes. (eg: not having open() essentially C&P)
Inheriting from io.RawIOBase means it can fit under/into the standard python buffered & text IO wrapper classes
I've manually removed/replaced/inlined use of a handful of internal library functions & test setup. If there's anything you get stuck on please ask.

dbaston · 2023-08-16T11:16:54Z

Fantastic, thank you!

dbaston · 2024-04-17T19:30:36Z

I have not forgotten about this. I'm leaning towards moving it to gdaltest to focus on internal use for now.

github-actions · 2024-05-16T02:36:24Z

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular link
to any issues which this pull request fixes
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in 2 weeks.

rouault · 2024-05-25T12:38:04Z

autotest/pymod/gdaltest.py

+        gdal.VSIFWriteL(x, 1, len(x), self._fp)
+
+    def seek(self, offset, whence=0):
+        gdal.VSIFSeekL(self._fp, offset, whence)


we need to be careful here. Due to VSIFSeekL() taking only unsigned offsets, negative seeking with SEEK_CUR must be emulated doing VSIFSeekL(fp, VSIFTellL(fp) + offset, SEEK_SET)

Where should the emulation happen?

VSIFile.seek

gdal.VSIFSeekL (via %pythonprepend%)

gdal.VSIFSeekL (via a new wrapper function in cpl_i. The signature here already uses signed offsets, so it seems reasonable that it should handle them correctly)

I'm still scratching my head a bit as to why the tests using negative seek work correctly as-is.

gdal.VSIFSeekL (via a new wrapper function in cpl_i. The signature here already uses signed offsets, so it seems reasonable that it should handle them correctly)

sounds good here. By the way, I've just realized yesterday, that is is not needed to use a "wrapper_foo" + %rename tricks. You can directly create a inline function with the target name. cf 5bcbc5c#diff-56ee4d91557b7ba46f850e46abbdc73e617c981c662a47a27d00eaef97136061R972

I'm still scratching my head a bit as to why the tests using negative seek work correctly as-is.

I suppose that the negative offsets get turned into unsigned values within SWIG .cpp code, and then unsigned overflow on additions within the virtual file system implementations accidentally lead to the correct result. But we'd better not rely on that. In particular the CI sanitize target and our configuration of OSSFuzz errors out on unsigned integer overflows, as they are most of the time bugs.

Added code to handle negative values in afe7ca8. I think the wrapper_ and %rename are needed in this case because the function name exposed by SWIG is the same as the underlying function name.

rouault · 2024-05-30T12:24:48Z

autotest/pymod/gdaltest.py

+            assert type(x) is str
+            x = x.encode(self._encoding)
+
+        gdal.VSIFWriteL(x, 1, len(x), self._fp)


we should probably assert the returned value to be len(x) ?

That makes sense, and to be consistent, we should check the return code of VSIFSeekL and throw an exception on failure?

we should check the return code of VSIFSeekL and throw an exception on failure?

sounds good

autotest/pymod/gdaltest.py

Co-authored-by: Even Rouault <even.rouault@spatialys.com>

rouault reviewed Aug 15, 2023

View reviewed changes

github-actions bot added the stale label May 16, 2024

dbaston added 2 commits May 21, 2024 09:24

autotest: Add a VSIFile helper class

44bb523

autotest: use gdaltest.vsi_open

6a5158c

dbaston force-pushed the python-vsifile-class branch 2 times, most recently from 8532886 to 6a5158c Compare May 21, 2024 13:26

dbaston changed the title ~~WIP: Python bindings: Add a VSIFile wrapper class~~ autotest: Add a VSIFile wrapper class May 21, 2024

github-actions bot removed the stale label May 22, 2024

rouault reviewed May 25, 2024

View reviewed changes

SWIG: Handle negative offsets in VSIFSeekL

8e7f2c9

dbaston force-pushed the python-vsifile-class branch from afe7ca8 to 8e7f2c9 Compare May 30, 2024 01:20

rouault reviewed May 30, 2024

View reviewed changes

VSIFile: Raise OSError for failed seek or write

e5f9781

rouault reviewed May 30, 2024

View reviewed changes

autotest/pymod/gdaltest.py Outdated Show resolved Hide resolved

Update autotest/pymod/gdaltest.py

3e747a9

Co-authored-by: Even Rouault <even.rouault@spatialys.com>

rouault added this to the 3.10.0 milestone May 30, 2024

rouault merged commit 5394ddf into OSGeo:master May 30, 2024
32 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autotest: Add a VSIFile wrapper class #8222

autotest: Add a VSIFile wrapper class #8222

dbaston commented Aug 14, 2023 •

edited

coveralls commented Aug 14, 2023 •

edited

rouault Aug 15, 2023

dbaston May 21, 2024

rouault Aug 15, 2023

dbaston May 21, 2024

rcoup commented Aug 16, 2023 •

edited

dbaston commented Aug 16, 2023

dbaston commented Apr 17, 2024

github-actions bot commented May 16, 2024

rouault May 25, 2024

dbaston May 29, 2024

rouault May 29, 2024

dbaston May 30, 2024

rouault May 30, 2024

dbaston May 30, 2024

rouault May 30, 2024

autotest: Add a VSIFile wrapper class #8222

autotest: Add a VSIFile wrapper class #8222

Conversation

dbaston commented Aug 14, 2023 • edited

What does this PR do?

What are related issues/pull requests?

Tasklist

coveralls commented Aug 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcoup commented Aug 16, 2023 • edited

dbaston commented Aug 16, 2023

dbaston commented Apr 17, 2024

github-actions bot commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaston commented Aug 14, 2023 •

edited

coveralls commented Aug 14, 2023 •

edited

rcoup commented Aug 16, 2023 •

edited