Skip to content

Conversation

@rbuffat
Copy link
Contributor

@rbuffat rbuffat commented May 20, 2020

This PR extends the functionality of MemoryFile and ZipMemoryFile:

  • Append support for MemoryFile
  • Write support for ZipMemoryFile
  • fiona.listdir() to list files in a directory
  • ZipMemoryFile.listdir() and ZipMemoryFile.listlayers() to list files and layers in a ZipMemoryFile

Not all drivers seem to support MemoryFiles. The tests are extended to use all drivers available on the CI. A mechanism similar to drvsupport.driver_mode_mingdal is introduced to guard not supported drivers and modes.

Issues:

  • MemoryFileBase.exists() seems to have issues for some drivers when the memory files are created without the proper extension (e.g. the Shapefile driver)
  • It would be preferable to use fiona.listdir() and fiona.listlayers() instead of ZipMemoryFile.listdir(), ZipMemoryFile.listlayers(), however, it is not that straightforward to create the path required as argument.

Relevant issues: #830 #754

This PR is probably best suited for Fiona 1.9. I created it against 1.8, as this branch includes a more complete drvsupport list. Once 1.9 catches up it should be possible to change the PR to 1.9.

@rbuffat rbuffat changed the title extend MemoryFile and ZipMemoryFile Extend MemoryFile and ZipMemoryFile May 20, 2020
@coveralls
Copy link

coveralls commented May 20, 2020

Coverage Status

Coverage increased (+0.1%) to 86.49% when pulling 3b94ecd on rbuffat:extend_memoryfile_support into 7559619 on Toblerity:maint-1.8.

@sgillies
Copy link
Member

@rbuffat yesterday I finished a refactor of rasterio's MemoryFile after it was reported that its seek method was broken, preventing unordered multipart writes: https://github.com/mapbox/rasterio/pull/1935/files. I'd like to do the same for fiona's MemoryFile. After which, let's try to overlay this PR on top, and then I'll see able copying the directory support to rasterio :)

@sgillies sgillies added this to the 1.9 milestone May 24, 2020
@rbuffat rbuffat force-pushed the extend_memoryfile_support branch from ce5cfbe to b021aca Compare May 26, 2020 11:51
@rbuffat rbuffat force-pushed the extend_memoryfile_support branch from 7b3e403 to ac0dd38 Compare May 26, 2020 13:08
@rbuffat
Copy link
Contributor Author

rbuffat commented May 26, 2020

@sgillies I will update this PR once you merge these changes.

I had a look at MemoryFileBase.exists(). It looks as CPLCheckForFile does more reliably detect the existence of a dataset when a name without extension is used. However, there are still a few issues:

  • CPLCheckForFile returns True also for none dataset files. As long we control the filenames this should be not an issue.
  • GMT driver for GDAL 1.x requires an extension despite using CPLCheckForFile. With a .gmt extentions it works. (I did not yet test OGR_GMT)
  • GPKG driver needs a file extension for GDAL 2.0 when appending. Otherwise GDAL uses the SQLite driver instead.

A solution would be to set the appropriate extension in the initializationMemoryFile. However, this is a bit tricky because at this point it is unknown which driver will be used (or even written with write()).

@rbuffat
Copy link
Contributor Author

rbuffat commented Aug 30, 2020

@sgillies I merged the new MemoryFile and tried to extend the test coverage using as many drivers as possible. This revealed some bugs and segfaults I need to look at (see TODOs in tests/test_memoryfile.py).

The most common issue is that drivers have problems opening filenames without the proper extension. Also, the OGR_GMT/, GMT seems to add ".gmt" to filenames without a proper extension. The extensions of a driver can be accessed using GDALGetMetadataItem(cogr_driver, DMD_EXTENSION, NULL) if the driver is known. GDALGetMetadataItem should be available for > GDAL 2.0.

I would suggest refactoring MemoryFile to use the proper extension for the filenames when possible (= the driver is known).

@sgillies
Copy link
Member

Thank you! That's super useful. Fiona should certainly respect the GDAL/OGR driver capability API.

@KeynesYouDigIt
Copy link

FWIW, ZipMemoryFile.listdir() and ZipMemoryFile.listlayers() would be really helpful, I came here to raise an issue but it looks like its already in processes? these methods would be helpful for me :)

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.

4 participants