Repository navigation
Conversation
…time download The check never rejected anything: `standard_name` was typed `CFStandardName | str | None`, so a name that failed the CF check fell through to `str` and was accepted. The only effect of the check was a ~4.5 MB download of the CF standard-name table every time eopf_geozarr was imported, which also makes `import eopf_geozarr` (and `eopf --help` via the CPM entry point) depend on the network. Remove `get_cf_standard_names`, `CF_STANDARD_NAME_URL`, the import-time fetch, `check_standard_name` and `CFStandardName`. Type `standard_name` as `str | None` on the S1 and S2 array attrs (same behaviour as before). Drop the CF tests and the `cf-xarray` dependency, whose only use was the table parser. Closes #265 Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Resolve the tests/test_data_api/test_geozarr/test_common.py conflict with #277: keep main's hand-built test_multiscales_round_trip() and drop the CF tests. uv.lock is main's lock minus cf-xarray. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Runs the import in a fresh interpreter with DNS and socket connect stubbed to raise a BaseException, so an except-OSError fallback cannot hide the call. With main's CF fetch restored, the import raises from get_cf_standard_names. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
| "socket.socket.connect = deny\n" | ||
| "import eopf_geozarr\n" | ||
| ) | ||
| subprocess.run([sys.executable, "-c", code], check=True) |
Contributor
There was a problem hiding this comment.
can we avoid subproicess?
Contributor
|
@lhoupert is this PR still valid? |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CF standard-name check never rejected anything:
standard_namewas typedCFStandardName | str | None, so a name that failed the check fell through tostr. Its only effect was downloading the ~4.5 MB CF table on everyimport eopf_geozarr. This removes the check, the import-time fetch and thecf-xarraydependency (its only use was the table parser).standard_nameis nowstr | None, which behaves as before. A new test checks thatimport eopf_geozarrmakes no network call; with the old fetch restored, it fails. Removal was agreed in #265 and in #277 (row 7). Closes #265.For reviewers:
uv.lockis main's lock minus cf-xarray, plus the editableeopf-geozarrversion set to 0.12.0 (main's lock was left at 0.11.0 by the release). It was hand-edited because a newer local uv rewrites many unrelated markers;uv lock --checkpasses.Author attestation
AI-assisted: implemented with Claude Code and checked by a separate review agent. pre-commit (ruff, format, pyright) and the full non-network suite (758 passed) pass locally.