Fix Brandl2020 downloading HTML error pages instead of data - #1144
Open
bruAristimunha wants to merge 2 commits into
Open
Fix Brandl2020 downloading HTML error pages instead of data#1144bruAristimunha wants to merge 2 commits into
bruAristimunha wants to merge 2 commits into
Conversation
DepositOnce migrated to DSpace 7. Its web host now answers every path -- including ones that used to be files -- with HTTP 200 and a 1306-byte Angular app shell rather than a 404, so pooch stored that HTML under each requested .mat name and recorded a successful download. All 16 subjects and the montage cached as byte-identical HTML, and nothing raised. The REST API is on a different host, api-depositonce.tu-berlin.de, named in the front end's own assets/config.json. It serves the bytes and honours Range requests. The per-file bitstream UUIDs needed to address it were already in this module but unused; the download path still built URLs against the broken host. Two changes: - Point downloads at the REST host, addressed by those UUIDs. - Verify what arrives. Every file here is MATLAB v5 or v7.3, both of which begin with the ASCII banner "MATLAB". A payload that does not is deleted and raises, because a cached bad file is worse than a failed download: every later call is served the same bytes without touching the network, so the failure surfaces at analysis time instead of at fetch time. data_dl gains an optional fname, since DSpace serves every bitstream from .../bitstreams/<uuid>/content and URL-derived naming would store them all as "content".
Review of the first commit raised three things: A bare RuntimeError is indistinguishable from the ones moabb raises for a missing unrar binary or an absent XDF stream, and base.py catches (RuntimeError, ValueError, OSError) broadly -- so a benchmark sweeping datasets could not tell "DepositOnce is down, skip it" from "moabb has a bug". Adds DatasetDownloadError beside the existing NemarDownloadError, for a download that succeeded but did not deliver the dataset's data. A stale bitstream UUID answers 404 with JSON, not 200 with HTML, so the payload check never sees it and the user gets a bare HTTPError naming an opaque UUID that appears nowhere in the dataset. That case is now caught and re-raised pointing at the stable handle, with the resolution command recorded next to the table it regenerates. The old message told the user to "retry later" -- an infinite loop for a permanent failure. It now says the failure is not transient. The URL test asserted the module formats its own constants, which cannot fail for the reason the table exists to worry about. Replaced with two properties that can: the table covers pp1-pp16 plus mnt.mat exactly, and no UUID repeats (a copy-paste slip would serve one subject's recording under another's name).
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
1 participant
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.
Fixes #1141.
The bug
Brandl2020downloaded 16 subject files and the montage as byte-identical 1306-byte HTML pages, and nothing raised. DepositOnce migrated to DSpace 7, and its web host now answers every path — including ones that used to be files — with HTTP 200 and the Angular app shell instead of a 404:Because the status is 200, pooch cached the error body under the
.matname and recorded a successful download. Every later call was then served that cache without touching the network, so the failure only surfaced when something tried to read the file.The fix
The REST API is on a different host. That is why probing
depositonce.tu-berlin.delooks like a total outage — even/server/apireturns the shell. The front end names its own API location inassets/config.json:That host serves the bytes and honours Range requests. The per-file bitstream UUIDs needed to address it were already in this module but never used —
_BITSTREAM_UUIDSwas dead code whiledata_pathstill built URLs against the broken host. This PR wires them up.And verifies what arrives. Every file here is MATLAB v5 or v7.3, both of which begin with the ASCII banner
MATLAB. A payload that does not is deleted and raises with an explanation. A cached bad file is worse than a failed download — it fails at analysis time, far from the cause.data_dlgains an optionalfname, because DSpace serves every bitstream from.../bitstreams/<uuid>/content; without it all 17 files would be stored ascontent.Verification
I downloaded the full distribution through this route: 19 files, 10.59 GiB, each checked against the size and checksum the API reports, all valid MATLAB.
New tests (parametrized) cover: the URL targets the API host with the right UUID,
fnameis passed so files are not all namedcontent, an HTML/empty/garbage payload raises and is removed from the cache, and an unknown filename is rejected.pytest moabb/tests/test_download.py moabb/tests/test_datasets.pypasses;ruff checkandruff formatare clean.Note for maintainers
Two things I deliberately left out of this PR:
bands.matandivals.matship with the distribution but are not fetched by this module. I did not add them, since nothing in the loader uses them — flagging in case that is unintentional.content-typecheck insidedata_dlwould protect every dataset against this class of failure, but it risks breaking downloads I cannot test, so it felt wrong to bundle with a bug fix. Happy to send it separately if you want it.