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 4 commits into
Conversation
Sevans711
left a comment
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
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).
|
|
||
| def test_parse_nod2d_missing_file_names_the_file(tmp_path): | ||
| """A missing 'nod2d.out' must say which file is missing and where.""" | ||
| from uxarray.io._fesom2 import _parse_nod2d |
There was a problem hiding this comment.
I would have a slight preference for imports like this to be at top of file, rather than within each test, for the test suite, especially for a small test suite file like this! I'm not sure what the benefit is of putting these imports directly inside the test functions.
There was a problem hiding this comment.
(This applies to a variety of imports throughout your new test functions, but I'm only leaving this one comment rather than making a new comment thread for each one.)
| otherwise read_file raises ImportError and the test would pass for the | ||
| wrong reason. | ||
| """ | ||
| import pytest |
There was a problem hiding this comment.
pytest import should definitely go at top of file; that would match the style of all other pytest imports throughout uxarray test suite.
| """Assuming WGS84 for CRS-less data is a guess and must be announced.""" | ||
| import pytest | ||
| gpd = pytest.importorskip("geopandas") | ||
| from shapely.geometry import Polygon |
There was a problem hiding this comment.
shapely import should probably go at top of file for pytest test suite, but isn't as necessary to move it. It would avoid the need to re-import in multiple tests at least.
| """ | ||
| import pytest | ||
|
|
||
| pytest.importorskip("pyogrio") |
There was a problem hiding this comment.
Don't introduce importorskip to the test suite; no other tests are using importorskip, and, like you mentioned in your comment, you expect pyogrio to be installed when uxarray has been installed with all dependencies. The test suite only gets run in an environment with all dependencies installed.
| """Dropping a geometry silently would yield a grid missing a face with no | ||
| indication that anything was skipped.""" | ||
| import pytest | ||
| gpd = pytest.importorskip("geopandas") |
There was a problem hiding this comment.
(See previous comments, don't introduce importorskip)
| # Try opening with xarray's default read engine | ||
| return xr.open_dataset(filename_or_obj, chunks=chunks, **kwargs) | ||
| except Exception: | ||
| except Exception as default_engine_error: |
There was a problem hiding this comment.
Apologies if you already clarified this in comment thread but could you clarify why this change is being added as part of this PR? (Might not need to change anything in response to my comment here, just not fully understanding yet how this helps to fix the original issue.)
| raise FileNotFoundError("TODO: ") | ||
| raise FileNotFoundError( | ||
| f"Expected a FESOM2 ASCII grid directory containing 'nod2d.out', " | ||
| f"but no such file exists under {grid_path!r}." |
There was a problem hiding this comment.
Minor suggestion: here and below, I recommend to use os.path.abspath(grid_path) in the error messages. Avoids confusion for any users with file structures like: run1/gridfolder/... and run2/gridfolder/... just showing "gridfolder" in the error message.
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.
2 participants
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.