Raise the underlying cause when geospatial data cannot be read - #1694
Raise the underlying cause when geospatial data cannot be read#1694rajeeja wants to merge 7 commits into
Conversation
Sevans711
left a comment
There was a problem hiding this comment.
Good find, I definitely agree with this premise of needing better error handling here; the version on main is not using best practices for error handling in a few different ways: (1) try multiple things at once; (2) catch error and print but don't re-raise, and (3) then continue with the code which will crash with confusing UnboundLocalError if an error occurred previously.
I am not 100% certain that GridInvalidError is the way to go here. What if the file simply doesn't exist, for example? Then it should be a FileNotFoundError. I think at least this case needs to be handled separately. Perhaps changing "except Exception" to "except ValueError" would be a cleaner way to do this. I tend to be skeptical of "except Exception" unless there is a clear reason for wanting to catch every error type.
More on that point, though: what is the motivation for catching errors here at all? Why not just allow gpd.read_file(...) to fail directly with its own error message?
The previous handler caught every read failure, printed it, and left gdf unbound, so the next line died with UnboundLocalError instead of the real cause. Wrapping the failure in GridInvalidError hid the backend's own exception type, and no other reader in uxarray/io wraps its backend's errors, so drop the try/except and let gpd.read_file raise. The underlying problem was packaging: geopandas was unpinned and releases before 1.0 do not require pyogrio, so an install could end up with geopandas and no file-IO backend at all, making every read_file call fail with ImportError. The regression test now skips without pyogrio, since it would otherwise pass on that ImportError rather than on an actual parse failure.
The geopandas reader and the netCDF fallback shared the same pattern as issue #1693: a failure was caught and reported to stdout, or replaced by a later one, so the actual cause never reached the caller. _open_dataset_with_fallback now chains the fallback engine's error onto the default engine's, so a file that neither engine can open reports both reasons rather than only the second. The two FESOM2 ASCII parsers raised FileNotFoundError("TODO: "), which named neither the missing file nor the directory searched. Assuming WGS84 for CRS-less geospatial data and skipping a geometry type the reader does not support are both warnings now. The latter silently produced a grid with a missing face, which is a wrong result rather than a diagnostic. _is_structured is called speculatively for every dataset before any other format check, so its stdout diagnostics fired for perfectly valid MPAS, Exodus and SCRIP files. They are removed; a negative result is the normal case and _parse_grid_type already raises an actionable error.
|
I hit this by accident — testing something unrelated and saw four failures in You're right that we shouldn't be catching here at all — none of the other readers do; Pinned |
Sevans711
left a comment
There was a problem hiding this comment.
Looks pretty reasonable as an overall set of changes to make, but I left a variety of inline comments to be addressed, including one point where I just needed a bit more clarification on the reason for the changes. Thank you for doing this work to improve the clarity and error handling throughout the code!
Flagging #1617 as related (may want to delay any work there on uxarray/io error messages until this merges, since this touches multiple errors in uxarray/io).
Move test-local imports to module scope, drop the importorskip calls now that the full test suite always runs with all dependencies installed, and report absolute paths in fesom2's missing-file errors so users with same-named subfolders under different run directories aren't confused by identical-looking relative messages.
Sevans711
left a comment
There was a problem hiding this comment.
Looks ready to merge, thank you for implementing this fix and for all the back and forth while addressing my comments!
Closes #1693
_gpd_readcaught every read failure, printed it, and fell through, leavinggdfunbound so the next line raisedUnboundLocalErrorinstead of the real cause; dropped the try/except entirely so geopandas raises directly (also surfacesFileNotFoundErrorfor free).geopandaswas unpinned and pre-1.0 releases don't requirepyogrio, so an install could end up with no file-IO backend at all; pinnedgeopandas>=1.0._open_dataset_with_fallbacknow chains the fallback engine's error onto the default engine's, so a file neither engine can open reports both reasons.FileNotFoundError("TODO: "); now name the missing file/directory.warnings.warninstead ofprint; the latter previously produced a grid silently missing a face._is_structured's speculative stdout diagnostics, which fired on every valid MPAS/Exodus/SCRIP file.