Skip to content

[BUGFIX] Download topography once in the tests, and fail fast when the GMT server is down - #217

Merged
mthielma merged 1 commit into
mainfrom
bk/topo-tests-download-once
Sep 30, 2026
Merged

mthielma merged 1 commit into
mainfrom
bk/topo-tests-download-once

Conversation

@boriskaus

Copy link
Copy Markdown
Member

Whats the purpose of this PR?

  • Bug fix
  • New feature
  • Documentation update
  • Other, please explain

Describe it in more detail below:

CI jobs on main have been running for two hours (and then failing) whenever the GMT data server throttled a runner; see the attempt-1 jobs of run 36567599426. A healthy run downloads only ~13 MB of tiles in ~10 s, so the volume is not the problem. Three things combined:

  1. The topo cache key is the hash of test/topo_prefetch.jl, which Download topography without GMT #216 changed, so all eleven matrix jobs started with an empty cache and hit the server at the same moment. Eight got through, three were throttled.
  2. Without the server index (gmt_data_server.txt), import_topo treated a tiled resolution as a single global file (earth_relief_01m_g.grd, which does not exist).
  3. Nothing remembered that the server was down: the index was re-requested three times per import_topo call, 4 mirrors × 60 s each. That is the exact 12m24 per testset in the log, ten calls per run.

Also: the scheduled run was not actually testing the download, because julia-actions/cache restores ~/.julia including scratch spaces, so the tiles were already there.

Library (src/import_topo.jl)

  • A copy of the server index is bundled in assets/gmt_data_server.txt (125 KB, changes rarely). The download is attempted once per session with a 15 s timeout; otherwise the bundled copy is used, so tile sizes, registration, scale and filler never depend on the network.
  • After a full round of mirrors fails, download_tile records the server as unreachable (a marker file in the cache, shared with the parallel test workers) and refuses further requests for 10 minutes with a clear error naming GeophysicalModelGenerator.reset_topo_server(). A timeout is no longer treated like a missing tile and silently filled with sea level.

Tests: download once, reuse everywhere

  • test/topo_prefetch.jl now lists every region/resolution any test or tutorial uses (the four La Palma sets from test_import_topo.jl were missing), so it is the only place that downloads; the tests read from the cache.
  • It records the outcome in ENV["GMG_TOPO_PREFETCH_OK"], which the workers inherit. test_import_topo.jl, test_GMT.jl, test_WaterFlow.jl and the La Palma tutorial skip their topography parts (@test_skip) if the server was unreachable; the scheduled run insists, so a real breakage is still caught.
  • New unit tests check the bundled index knows the tiled sets.

CI

  • restore-keys prefix, so a changed key still restores the previous tiles and only new ones are fetched.
  • The scheduled run deletes the tile directory before testing, so the download is genuinely exercised every two weeks.
  • .raw/.part/marker files excluded from the cache; timeout-minutes: 90 as a backstop.

Worst case now if the server is down on a PR: ~10 minutes for the prefetch's one attempt, then everything else fails or skips instantly, instead of two hours.

Verified locally: cached path (0 downloads, import_topo 32/32 in 2 s) and a simulated outage (unreachable mirrors, empty cache: first call errors after one mirror round, second call refused in 10 ms, download tests recorded as skipped).

Checklist

  • The PR title is descriptive and starts with the appropriate tag: [BUGFIX], [ADDITION], [DOC], etc.
  • New tests (either assessing the correct behaviour of new internal functions or the correctness of a tutorial) were added, or old tests were updated
  • Affected tutorials have also been updated (none affected)
  • The new feature was added in a way that does not break public API
  • New documentation related to the new feature was added (docstrings)
  • The new code follows the contributor guidelines, in particular the Runic Style

🤖 Generated with Claude Code

…ver is down

CI jobs on main have been running for two hours whenever the GMT data server
throttled a runner. Three things combined: the topo cache key changed with the
prefetch list, so every matrix job started with an empty cache and hit the
server at once; without the server index, `import_topo` looked for a tiled
resolution as a single global file that does not exist; and nothing remembered
that the server was down, so the index was re-requested three times per call
(4 mirrors x 60 s each) -- 12 minutes per testset, ten calls per run.

Library:
- Bundle a copy of gmt_data_server.txt in assets/. The index is downloaded once
  per session (15 s timeout) and the bundled copy is used otherwise, so tile
  sizes, registration, scale and filler never depend on the network.
- After a full round of mirrors fails, `download_tile` records the server as
  unreachable (marker file in the cache, shared with the test workers) and
  refuses further requests for 10 minutes with a clear error; a timeout is no
  longer treated like a missing tile and silently filled with sea level.
  `reset_topo_server()` clears the marker.

Tests:
- topo_prefetch.jl lists every region/resolution the tests and tutorials use,
  so it is the only place that downloads; the tests read from the cache.
- It records the outcome in ENV["GMG_TOPO_PREFETCH_OK"]; the topography tests
  skip themselves when the server was unreachable, except on the scheduled run,
  which insists so a real breakage is caught.

CI:
- restore-keys so a changed key still restores the previous tiles.
- The scheduled run clears the tile cache first: julia-actions/cache restores
  scratch spaces too, so the download was not actually being tested.
- Exclude decoded .raw files from the cache; 90 minute job timeout.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mthielma
mthielma self-requested a review September 30, 2026 07:19

@mthielma mthielma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test now do not run for more than 30 minutes, so this seems to work.

@mthielma
mthielma merged commit cded7b4 into main Sep 30, 2026
20 checks passed
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.

2 participants