Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5556 +/- ##
=========================================
Coverage 61.53% 61.53%
Complexity 14 14
=========================================
Files 2 2
Lines 78 78
Branches 4 4
=========================================
Hits 48 48
Misses 24 24
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
4e73252 to
6da2fa4
Compare
DanySK
left a comment
DanySK
left a comment
There was a problem hiding this comment.
There are several critical issues that must be solved before I go ahead with the review. See comments.
fbd2faa to
8337419
Compare
|
Hi @Wanes01! 👋 |
1 similar comment
|
Hi @Wanes01! 👋 |
8e54f3b to
8a18c40
Compare
|
|
Hi @Wanes01! 👋 |
8a18c40 to
08289e1
Compare
DanySK
changed the title
|
@codex review |
4bff9bb to
6af35f1
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
DanySK
left a comment
DanySK
left a comment
There was a problem hiding this comment.
Review focused on correctness and idiomatic, readable Kotlin. One inline comment per issue.
| private val incarnation = mockk<Incarnation<Any?, GeoPosition>>() | ||
|
|
||
| @Test | ||
| fun `OSMEnvironment pbf import and DoubleCopernicusLayer must coexist`() { |
There was a problem hiding this comment.
This test looks redundant now: TestCopernicusExamples loads local-data.yml/.kts, which builds an OSMEnvironment from maps/cesena.pbf together with a DoubleCopernicusLayer and checks the perceived values, so the OSM + cdm-core coexistence is already exercised there through the DSL. This one does it by hand (MockK Incarnation, manual temp dir with try/finally, an empty line inside the body on line 51). I would remove it. If you keep it, build the scenario through the DSL and reuse the copernicus/glofas-cesena resource instead of writing a NetCDF file.
| to download the data again, for instance because the data store published a revised version, | ||
| delete the corresponding entry from the cache directory. | ||
|
|
||
| ### Other parameters |
There was a problem hiding this comment.
checkMd5 is listed under "Other parameters", which suggests it is optional, but it has no default in either class and must always be given (both examples pass checkMd5: true). Please say it is required, or give it a default in the code (see the comment on DoubleCopernicusLayer).
| Data slices are timestamped with real-world instants, while simulations advance in simulation time units. | ||
| Two parameters, both written in ISO-8601, connect them: | ||
|
|
||
| * `timeScale` is the real-world duration of one simulation time unit (by default, `PT1H`: one hour); |
There was a problem hiding this comment.
The defaults described here (PT1H, default cache directory, ~/.cdsapirc, PT30M) exist only on DoubleCopernicusLayer. The generic CopernicusLayer, which line 165 recommends for other types, requires every parameter. Either qualify the defaults ("for DoubleCopernicusLayer") or move the defaults to CopernicusLayer (preferred, see the code comment).
|
|
||
| ## Interpolation and conversion | ||
|
|
||
| Named parameters are supported in both YAML and the Kotlin DSL; |
There was a problem hiding this comment.
This note about named parameters is general, not specific to interpolation/conversion, but it sits under "Interpolation and conversion", while the place where users hit it is the datastore example (timeOrigin: null, variable: null just to reach cacheDirectory). Suggest moving it next to the datastore example or into the "Other parameters" section. Small wording fix on line 63: "request the data directly from a data store".
| All the files in the directory must hold the same variable on the same grid, | ||
| and their time slices get merged in chronological order | ||
| (if several files hold the same instant, the slice of the first file is used). | ||
| The directory can be a path on the file system or the name of a directory among the classpath resources. |
There was a problem hiding this comment.
"The name of a directory among the classpath resources" only works when resources are unpacked on disk (e.g. build/resources). From inside a JAR, Path.of(jar: URI) fails (see the comment on CopernicusLayer.localPath). Please qualify this, or fix the code.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Remove generated GRIB index artifacts
The added .gbx9 and .ncx4 files are NetCDF-Java-generated indexes (the reader itself identifies these suffixes as indexes), and they embed the creator-specific path /home/wanes/Desktop/.../fc.grib. Keeping them in test resources makes the fixture machine-specific and risks CDM consuming stale index metadata; delete both indexes and let tests regenerate them under build output.
AGENTS.md reference: AGENTS.md:L44-L44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val request = CopernicusRequest(endpoint.trim().trimEnd('/'), dataset, inputs) | ||
| return FileSystemDirectoryCache(provider, cacheDirectoryRoot)[request] |
There was a problem hiding this comment.
When the same shared cache is first populated with checkMd5=false (or that loader wins a concurrent race), this request receives exactly the same cache entry as a later checkMd5=true load. FileSystemDirectoryCache returns that existing directory without invoking the provider, so the later load silently skips its requested checksum verification; even an asset with the advertised size but bad contents, or one whose advertised checksum should fail, is treated as verified. Preserve verification metadata and upgrade/reload unverified hits, or otherwise keep the two cache states distinct.
Useful? React with 👍 / 👎.
| * @throws IllegalArgumentException if [path] is neither an existing path nor a classpath resource. | ||
| */ | ||
| private fun localPath(path: String): Path = Path.of(path).takeIf { it.exists() } | ||
| ?: ResourceLoader.getResource(path)?.let { Path.of(it.toURI()) } |
There was a problem hiding this comment.
When dataDirectory or inputsFile is a classpath resource inside a packaged JAR, ResourceLoader returns a jar: URL and Path.of(it.toURI()) throws FileSystemNotFoundException unless that JAR filesystem was explicitly mounted. Thus the newly advertised classpath-resource flow works in the exploded Gradle test classpath but fails in packaged distributions; copy the resource to a real directory/file or explicitly manage the archive filesystem.
Useful? React with 👍 / 👎.
| .uri(URI.create(url)) | ||
| .header("PRIVATE-TOKEN", token) | ||
| .header("Accept", APPLICATION_JSON) | ||
| .GET() | ||
| .build(), |
There was a problem hiding this comment.
When a monitor endpoint accepts the connection but never completes its response, this request has no HttpRequest.timeout, so http.send can block indefinitely and the deadline check in awaitSuccess is never reached. The configured provider timeout therefore is not a guillotine for a stalled poll; each polling request should be bounded by the remaining overall timeout.
Useful? React with 👍 / 👎.
| val previous = System.getProperty(SAX_PARSER_FACTORY_KEY) | ||
| System.setProperty(SAX_PARSER_FACTORY_KEY, JDK_XERCES_SAX_PARSER_FACTORY) | ||
| try { | ||
| return NetcdfDatasets.openDataset(file.toString()) |
There was a problem hiding this comment.
When two EagerGridSnapshots instances open files concurrently, these save/set/restore operations race on a process-wide system property: one call can restore the original parser while the other is still opening a GRIB, and the later call can finally leave the JDK parser installed instead of the original value. This can both break the concurrent GRIB read and corrupt SAX configuration for unrelated code, so the entire override/open/restore sequence needs a shared lock (or a non-global parser configuration).
Useful? React with 👍 / 👎.
| check(deadline.hasNotPassedNow()) { | ||
| "Timeout ($timeout) while waiting for job completion at $monitorUrl" | ||
| } | ||
| Thread.sleep(interval.inWholeMilliseconds) |
There was a problem hiding this comment.
When timeout is shorter than the current backoff interval, the code checks the deadline and then sleeps for the full interval; for example, a 500 ms timeout with the default 2 s interval waits 2 s and issues another GET before it can fail. This makes the configured timeout overrun by up to maxPollInterval even with responsive servers, so sleep only for the remaining duration and check the deadline before the next request.
Useful? React with 👍 / 👎.
| this.size <= 1 -> true | ||
| else -> { | ||
| for (i in 0 until this.size - 1) { | ||
| if (this[i] >= this[i + 1]) return false |
There was a problem hiding this comment.
When either axis contains NaN, the >= comparison is false rather than detecting an ordering violation (and arrays of length one bypass the loop entirely), so construction accepts an axis that is not strictly ordered. Later range checks and binary searches then reject valid positions or select bogus cells instead of failing at ingestion; validate each coordinate as non-NaN while checking adjacent values.
Useful? React with 👍 / 👎.
…cache API and default to the platform user cache directory via appdirs
6af35f1 to
e3ea446
Compare
|
No description provided.