From f0b1b872c390f8ea4a9b9fb22245a1f696894b46 Mon Sep 17 00:00:00 2001 From: arthurmccray Date: Wed, 9 Sep 2026 21:49:10 -0700 Subject: [PATCH] updating Add Dataset for links --- .github/ISSUE_TEMPLATE/new_dataset.yaml | 6 +- .github/scripts/fill_download_fields.py | 108 +++++++++++ .github/scripts/issue_to_yaml.py | 29 ++- .github/workflows/fill_download_fields.yml | 92 +++++++++ CONTRIBUTING.md | 9 +- README.md | 13 ++ docs/source/_build_docs.py | 183 ++++++++++++++---- docs/source/contributing.rst | 51 ++++- emdatabase/new_dataset.py | 87 ++++++++- emdatabase/tests/test_fill_download_fields.py | 142 ++++++++++++++ emdatabase/tests/test_forms.py | 151 +++++++++++++-- emdatabase/tests/test_new_dataset.py | 147 ++++++++++++++ 12 files changed, 949 insertions(+), 69 deletions(-) create mode 100644 .github/scripts/fill_download_fields.py create mode 100644 .github/workflows/fill_download_fields.yml create mode 100644 emdatabase/tests/test_fill_download_fields.py diff --git a/.github/ISSUE_TEMPLATE/new_dataset.yaml b/.github/ISSUE_TEMPLATE/new_dataset.yaml index 0300544..019521f 100644 --- a/.github/ISSUE_TEMPLATE/new_dataset.yaml +++ b/.github/ISSUE_TEMPLATE/new_dataset.yaml @@ -46,7 +46,7 @@ body: id: url attributes: label: --URL-- - description: Please provide a URL where the file can be downloaded. A link ending in the file name is split into the directory it is served from and the name; a link that names no file - a Google Drive `uc?export=download&id=` link, or anything else with a query string - is kept whole, and then the file name has to be given below. + description: Please provide a URL where the file can be downloaded. A link ending in the file name is split into the directory it is served from and the name; a link that names no file - a Google Drive `uc?export=download&id=` link, or anything else with a query string - is kept whole, and then the file name has to be given below. A Google Drive share link, `file/d//view` or `open?id=`, works too - it is rewritten to the download link that serves the file. placeholder: ex. https://zenodo.org/records/15490547/files/smallPtychography.hspy validations: required: true @@ -62,10 +62,10 @@ body: id: checksum attributes: label: --Checksum-- - description: Please provide the checksum of the dataset file for verification purposes. Include the type of checksum (e.g., md5, sha256) followed by the checksum value. + description: The md5 of the file, as `md5:<32 hex chars>`. Optional. Filled in automatically on the pull request by downloading the file; for a very large file, use the Add Dataset form's file picker instead. The size is filled in the same way. placeholder: ex. md5:df9376d5c020a23f0f7f51cfe79f303f validations: - required: true + required: false - type: textarea id: description attributes: diff --git a/.github/scripts/fill_download_fields.py b/.github/scripts/fill_download_fields.py new file mode 100644 index 0000000..ad87c75 --- /dev/null +++ b/.github/scripts/fill_download_fields.py @@ -0,0 +1,108 @@ +"""Fill in the ``checksum`` and ``size_bytes`` an index entry is missing. + +Run by ``.github/workflows/fill_download_fields.yml`` on a pull request that +touches ``emdatabase/index/``. The docs form and the issue form both let those +two fields be blank - a contributor cannot be asked to md5 a 100 GB file by hand +- while every entry needs both, so the file is downloaded here and whatever is +missing is computed from it by +:func:`~emdatabase.new_dataset.fill_download_fields`. + +A file is only written when something was filled in, and only after it passes +:func:`~emdatabase.metadata.validate_document` - the same check the test suite +and ``emdatabase.new_dataset`` run. A link that answers with ``text/html`` +served a page rather than the file; nothing is written for it and the run exits +non-zero. +""" + +from __future__ import annotations + +import argparse +from pathlib import Path + +import yaml + +from emdatabase.metadata import ( + INDEX_DIR, + NON_DATASET_FILES, + dataset_files, + validate_document, +) +from emdatabase.new_dataset import build_document, fill_download_fields, write_document + + +def index_files(index_dir: Path | None, paths: list[Path]) -> list[Path]: + """Every dataset YAML to fill in: the ones named, or a whole directory. + + ``vendors.yaml`` and the rest of ``index/`` are not dataset collections, so + they are dropped however they arrived - the workflow passes whichever files + the pull request changed. + """ + if paths: + return [path for path in paths if path.name not in NON_DATASET_FILES] + if index_dir is None: + return dataset_files() + return sorted(p for p in index_dir.rglob("*.y*ml") if p.name not in NON_DATASET_FILES) + + +def fill_file(path: Path) -> tuple[list[str], bool]: + """Fill one index file in; ``(summary lines, whether it went cleanly)``. + + The entries are rebuilt through + :func:`~emdatabase.new_dataset.build_document` before they are written, so a + field that was missing altogether lands in the shipped key order rather than + at the end of the entry. + """ + document = yaml.safe_load(path.read_text(encoding="utf-8")) + try: + lines = fill_download_fields(document) + except (OSError, ValueError) as error: + return [f"- **{path}**: {error}"], False + if not lines: + return [], True + + rewritten = {name: build_document(name, entry)[name] for name, entry in document.items()} + problems = validate_document(rewritten, origin=path) + if problems: + return [f"- **{problem}**" for problem in problems], False + write_document(path, rewritten) + return [f"- {line}" for line in lines] + [f"- wrote `{path}`"], True + + +def _parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + prog="fill_download_fields.py", + description="Download each index entry that is missing its checksum or size.", + ) + parser.add_argument( + "paths", nargs="*", type=Path, help="dataset YAML to fill in; default is every file" + ) + parser.add_argument( + "--index", + type=Path, + help=f"directory of dataset YAML to fill in (default {INDEX_DIR})", + ) + parser.add_argument("--summary", type=Path, help="write a markdown report of the run here") + return parser + + +def main(argv: list[str] | None = None) -> int: + args = _parser().parse_args(argv) + + lines: list[str] = [] + ok = True + for path in index_files(args.index, args.paths): + one, one_ok = fill_file(path) + lines += one + ok &= one_ok + if not lines: + lines = ["Every entry already has its checksum and size."] + + summary = "\n".join(lines) + print(summary) + if args.summary: + args.summary.write_text(f"{summary}\n", encoding="utf-8") + return 0 if ok else 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/scripts/issue_to_yaml.py b/.github/scripts/issue_to_yaml.py index 2bcf4b4..6d726f7 100644 --- a/.github/scripts/issue_to_yaml.py +++ b/.github/scripts/issue_to_yaml.py @@ -6,6 +6,10 @@ ``emdatabase.metadata.validate_document`` before it is written - the same check the test suite and ``emdatabase.new_dataset`` run - so a malformed issue fails here rather than in the pull request the workflow opens. + +The form's checksum is optional, and the size is only what a HEAD request said, +so ``emdatabase.new_dataset.fill_download_fields`` downloads the file for +whichever of the two the issue left blank before any of that. """ import re @@ -17,6 +21,8 @@ as_weights_family, build_document, content_length, + fill_download_fields, + normalize_url, split_url, version_date, write_document, @@ -95,8 +101,10 @@ def build_yaml(data): sys.exit("the issue has no dataset name") # The form asks for the download link; the YAML wants the directory and the # file name separately, and keeps the whole link as `url` only when the file - # is not served at `source/file`. - url = data["URL"].rstrip("/") + # is not served at `source/file`. A Drive share link is rewritten to the + # download link before either, so the HEAD request below asks about the file + # rather than the viewer page. + url = normalize_url(data["URL"].rstrip("/")) source, filename, link = split_url(url) if not source: sys.exit(f"{data['URL']!r} is not a link to a file") @@ -139,15 +147,24 @@ def build_yaml(data): return build_document(name, entry), name -if __name__ == "__main__": - issue_file, out_dir = sys.argv[1], Path(sys.argv[2]) +def write_yaml(issue_file, out_dir): + """Parse one issue body and write the entry it describes into ``out_dir``.""" document, dataset_name = build_yaml(parse_issue_body(Path(issue_file).read_text())) - out_path = out_dir / f"{dataset_name}.yaml" + out_path = Path(out_dir) / f"{dataset_name}.yaml" + for line in fill_download_fields(document): + print(line) + # A field the issue left blank is missing from the entry rather than at the + # end of it, so the document is rebuilt into the shipped key order. + document = {name: build_document(name, entry)[name] for name, entry in document.items()} problems = validate_document(document, origin=out_path) for problem in problems: print(problem) if problems: sys.exit("fix the issue and reopen it") - write_document(Path(out_path), document) + write_document(out_path, document) print(f"wrote {out_path}") + + +if __name__ == "__main__": + write_yaml(sys.argv[1], Path(sys.argv[2])) diff --git a/.github/workflows/fill_download_fields.yml b/.github/workflows/fill_download_fields.yml new file mode 100644 index 0000000..a6341ec --- /dev/null +++ b/.github/workflows/fill_download_fields.yml @@ -0,0 +1,92 @@ +name: fill download fields + +# The docs form and the issue form both let the checksum and the size be blank, +# because a contributor cannot be asked to md5 a 100 GB file by hand, while the +# test suite requires both on every entry. This job downloads whatever a pull +# request left blank, fills it in and pushes the result back to the branch. A +# fork's branch cannot be pushed to, so a pull request from one is failed with +# the values it would have written instead. +on: + pull_request: + paths: + - emdatabase/index/*.yaml + workflow_dispatch: + +permissions: + contents: write + pull-requests: write + +jobs: + fill: + name: checksum and size are filled in + runs-on: ubuntu-latest + # The files are whole datasets, so a slow host can take hours. + timeout-minutes: 120 + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.ref }} + repository: ${{ github.event.pull_request.head.repo.full_name }} + token: ${{ secrets.EMDATABASE_PAT || secrets.GITHUB_TOKEN }} + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install dependencies and package + run: pip install -U -e .'[dev]' + + # The checkout is the head repository, which for a fork carries its own + # idea of the base branch, so the base is fetched from this repository. + - name: Fetch the base branch + if: github.event_name == 'pull_request' + run: | + git fetch --no-tags https://github.com/${{ github.repository }} \ + +refs/heads/${{ github.base_ref }}:refs/remotes/origin/${{ github.base_ref }} + + - name: Fill in the checksum and size + run: | + if [ "${{ github.event_name }}" = "pull_request" ]; then + files=$(git diff --name-only origin/${{ github.base_ref }}... -- 'emdatabase/index/*.yaml') + if [ -z "$files" ]; then + echo "no index file changed" | tee filled.md + exit 0 + fi + python .github/scripts/fill_download_fields.py --summary filled.md $files + else + python .github/scripts/fill_download_fields.py --summary filled.md \ + --index emdatabase/index + fi + + - name: Push the filled-in entries + if: >- + github.event_name == 'pull_request' + && github.event.pull_request.head.repo.full_name == github.repository + run: | + if git diff --quiet -- emdatabase/index; then + echo "nothing to fill in" + exit 0 + fi + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + git commit -m "Fill in checksum and size" -- emdatabase/index + git push origin HEAD:${{ github.event.pull_request.head.ref }} + + - name: Report what a fork has to fill in itself + if: >- + github.event_name == 'pull_request' + && github.event.pull_request.head.repo.full_name != github.repository + run: | + if git diff --quiet -- emdatabase/index; then + echo "nothing to fill in" + exit 0 + fi + cat filled.md + git diff -- emdatabase/index + echo "This pull request comes from a fork, so these fields cannot be pushed to its" + echo "branch. Copy the values above into the entry, or fill them in yourself with" + echo "the Add Dataset form's file picker, and check the file with:" + echo " python -m emdatabase.new_dataset --validate " + exit 1 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 70aceb0..6096665 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,7 +2,10 @@ A dataset is one YAML file in `emdatabase/index/`. To add one, either fill in the [new-dataset issue form](https://github.com/electronmicroscopy/emdatabase/issues/new?template=new_dataset.yaml), -which opens the pull request for you, or run +which opens the pull request for you, or the +[Add Dataset form](https://electronmicroscopy.github.io/emdatabase/add_dataset.html), +which takes one download link - a Google Drive share link included - and fills in the +file name, size and md5 from your local copy of the file. Or run ```bash python -m emdatabase.new_dataset https://zenodo.org/records//files/ @@ -11,4 +14,8 @@ python -m emdatabase.new_dataset https://zenodo.org/records//files/. diff --git a/README.md b/README.md index 056e3cb..56f4d6d 100644 --- a/README.md +++ b/README.md @@ -192,6 +192,19 @@ to one already on the list fails CI as a misspelling. A technique close to one i `techniques.yaml` fails the same way. Open an issue with the [new dataset template](https://github.com/electronmicroscopy/emdatabase/issues/new?template=new_dataset.yaml), +fill in the [Add Dataset form](https://electronmicroscopy.github.io/emdatabase/add_dataset.html), or run `python -m emdatabase.new_dataset `, which fetches the checksum and size, prompts for the rest and writes the file for you to open a pull request with. See [CONTRIBUTING.md](CONTRIBUTING.md). + +All three take one download link and split it into `source`, `file` and, when the file +is not served at `source/file`, `url`. A Google Drive share link - what the share +button copies - is rewritten to the `uc?export=download&id=` link that serves the +file. The form also has a local file picker: point it at the copy on your machine and +it fills in the file name, size and md5, hashing the file in the browser without +uploading it. + +Neither web route needs the checksum or the size. A pull request carrying an entry +that is missing either one has the file downloaded on GitHub and the fields filled in +and pushed back to the branch; a pull request from a fork, whose branch cannot be +pushed to, fails with the values to paste in instead. diff --git a/docs/source/_build_docs.py b/docs/source/_build_docs.py index 16e56b7..4e01297 100644 --- a/docs/source/_build_docs.py +++ b/docs/source/_build_docs.py @@ -853,7 +853,8 @@ def _app_page( """Wrap page ``body`` in the self-contained Catppuccin app shell. Built by concatenation (not ``str.format``/``%``) so CSS/JS braces need no - escaping. The result references no external hosts - all CSS/JS is inline. + escaping. All the CSS is inline, and so is every script but the ones a page + passes in ``scripts`` itself. """ return ( "\n" @@ -1000,10 +1001,26 @@ def generate_weights_html() -> str: _TECHNIQUE_GROUPS = (("Acquisition", acquisition_techniques()), ("ML task", ml_tasks())) _KINDS = ("dataset", "weights") +# Neither field is required: whatever a contributor leaves blank is downloaded +# and filled in by .github/workflows/fill_download_fields.yml. +_FILLED_IN = ( + "Optional. Filled in automatically on the pull request by downloading the file; " + "for a very large file, pick the local file above instead." +) + # Owner/repo the prefilled "create new file" PR link targets. _REPO = "electronmicroscopy/emdatabase" _BRANCH = "main" +# md5 in the browser, for the Add Dataset page's local-file picker - the one +# external resource any generated page loads. If it does not load the picker +# still fills in the file name and size, and says so. +_SPARK_MD5_SRC = ( + '' +) + def _text_field(fid, label, required=False, placeholder="", hint="", full=False): req = ' *' if required else "" @@ -1122,6 +1139,20 @@ def _datalist_field(fid, label, options, placeholder="", hint=""): ) +def _file_field(fid, label, hint=""): + """A local file picker. Its hint carries an id, so the JS can show progress.""" + return ( + '
" + '
' + _esc(hint) + "
" + '' + '
' + ) + + def _author_row_html(): return ( '
' @@ -1155,41 +1186,35 @@ def generate_add_dataset_html() -> str: '' '
' + _text_field( - "f-source", - "Source URL", + "f-link", + "Download link", required=True, - placeholder="https://zenodo.org/records/15490547/files", - hint="Direct download base (no file name).", - ) - + _text_field( - "f-url", - "Download URL", - placeholder="https://drive.google.com/uc?export=download&id=", - hint=( - "Only when the download link is not / - a Google Drive " - "uc?export=download&id= link, or anything else with a query string. " - "File stays the name the file is saved under." - ), + placeholder="https://zenodo.org/records/15490547/files/smallPtychography.hspy", + hint="A direct link to the file. A Google Drive share link works.", full=True, ) + + _file_field( + "f-localfile", + "Local file", + hint="Pick the file you uploaded to fill in the name, size and md5.", + ) + _text_field( "f-checksum", "Checksum", placeholder="md5:df9376d5c020a23f0f7f51cfe79f303f", - hint="md5:<32 hex chars>", + hint=f"md5:<32 hex chars>. {_FILLED_IN}", ) + _text_field( "f-file", "File", - required=True, placeholder="smallPtychography.hspy", - hint="The file name at that source.", + hint="The name the file is saved under; needed when the link does not end in it.", ) + _text_field( "f-size_bytes", "Size (bytes)", placeholder="1104287335", - hint="The file's Content-Length, in bytes.", + hint=f"The file's Content-Length, in bytes. {_FILLED_IN}", ) + _datalist_field( "f-detector_manufacturer", @@ -1274,8 +1299,9 @@ def generate_add_dataset_html() -> str: "

Fill in the metadata; the YAML builds live on the right. " "“Open a Pull Request” sends you to GitHub with the new file " "pre-filled — commit it to a branch there and GitHub opens the PR.

" - "

From a terminal, python -m emdatabase.new_dataset <url> " - "fills in the checksum and size for you; see " + "

Pick the file from your machine to fill in its name, size and md5. " + "From a terminal, python -m emdatabase.new_dataset <url> " + "does the same from the link; see " 'Contributing a Dataset.

' "" '
' @@ -1316,7 +1342,7 @@ def generate_add_dataset_html() -> str: ) js = _ADD_DATASET_JS.replace("__REPO__", _REPO).replace("__BRANCH__", _BRANCH) - scripts = "" + scripts = _SPARK_MD5_SRC + "\n" return _app_page( "Add Dataset · EM-Database", body, @@ -1338,6 +1364,30 @@ def generate_add_dataset_html() -> str: return String(value == null ? "" : value).replace(/[^A-Za-z0-9_]+/g, ""); } +// A Google Drive share link as its download link; any other link unchanged. +// The port of new_dataset.normalize_url - the two have to agree, and a test +// runs the same links through both. +function emdbNormalizeUrl(url) { + var m = /^https?:\/\/drive\.google\.com\/file\/d\/([^/?#]+)/i.exec(url) + || /^https?:\/\/drive\.google\.com\/open\?(?:[^#]*&)?id=([^&#]+)/i.exec(url); + return m ? "https://drive.google.com/uc?export=download&id=" + m[1] : url; +} + +// `{source, file, url}` for a link, with `url` empty when unneeded. The port of +// new_dataset.split_url, down to what each branch returns. +function emdbSplitUrl(url) { + url = emdbNormalizeUrl(String(url == null ? "" : url)); + var m = /^([A-Za-z][A-Za-z0-9+.\-]*):\/\/([^/?#]*)([^?#]*)(?:\?([^#]*))?/.exec(url); + if (!m || !m[2]) return { source: "", file: "", url: "" }; + var path = m[3] || "", query = m[4] || ""; + var last = path.slice(path.lastIndexOf("/") + 1); + if (query || last.indexOf(".") === -1) { + return { source: m[1].toLowerCase() + "://" + m[2], file: "", url: url }; + } + var cut = url.lastIndexOf("/"); + return { source: url.slice(0, cut), file: url.slice(cut + 1), url: "" }; +} + // Today as YYMMDD, the label a new weights version is filed under. function emdbVersionDate() { var d = new Date(); @@ -1376,13 +1426,17 @@ def generate_add_dataset_html() -> str: // A weights entry writes these inside `latest` and its dated version instead. var weights = get("kind") === "weights"; var bytes = get("size_bytes").replace(/[^0-9]/g, ""); + // The form asks for one download link; the YAML wants the directory and the + // file name apart, and keeps the whole link only when it is not source/file. + var split = emdbSplitUrl(get("link")); + var file = get("file") || split.file; add("description", get("description")); - add("source", get("source")); + add("source", split.source); if (!weights) { - add("url", get("url")); + add("url", split.url); add("checksum", get("checksum")); } - add("file", get("file")); + add("file", file); if (bytes && !weights) lines.push(" size_bytes: " + bytes); add("detector_manufacturer", get("detector_manufacturer")); add("detector", get("detector")); @@ -1425,7 +1479,7 @@ def generate_add_dataset_html() -> str: }); } var pin = [ - ["url", get("url") || (get("source") + "/" + get("file"))], + ["url", split.url || (split.source + "/" + file)], ["checksum", get("checksum")] ].filter(function (pair) { return pair[1]; }); if (bytes) pin.push(["size_bytes", bytes]); @@ -1445,7 +1499,12 @@ def generate_add_dataset_html() -> str: } if (typeof module !== "undefined" && module.exports) { - module.exports = { emdbBuildYaml: emdbBuildYaml, emdbEntryName: emdbEntryName }; + module.exports = { + emdbBuildYaml: emdbBuildYaml, + emdbEntryName: emdbEntryName, + emdbNormalizeUrl: emdbNormalizeUrl, + emdbSplitUrl: emdbSplitUrl + }; } """ @@ -1460,15 +1519,21 @@ def generate_add_dataset_html() -> str: var addAuthor = document.getElementById("add-author"); var authorsBox = document.getElementById("authors"); var modelGroup = document.getElementById("model-group"); + var localFile = document.getElementById("f-localfile"); + var localHint = document.getElementById("hint-f-localfile"); var SCALARS = [ - "name", "description", "source", "url", "checksum", "file", "size_bytes", + "name", "description", "link", "checksum", "file", "size_bytes", "detector_manufacturer", "detector", "microscope_vendor", "microscope_model", "camera_length", "voltage", "license", "doi", "kind", "version_date", "model_class", "model_framework", "model_quantem" ]; function val(id) { var e = document.getElementById(id); return e ? e.value.trim() : ""; } + function setValue(id, value) { var e = document.getElementById(id); if (e) e.value = value; } + + // The CLI strips a trailing slash off the link before splitting it; so does this. + function link() { return val("f-link").replace(/\/+$/, ""); } function authors() { var out = []; @@ -1488,6 +1553,7 @@ def generate_add_dataset_html() -> str: function collect() { var fields = {}; SCALARS.forEach(function (key) { fields[key] = val("f-" + key); }); + fields.link = link(); fields.technique = Array.prototype.map.call( document.querySelectorAll("#f-technique input:checked"), function (box) { return box.value; } @@ -1515,14 +1581,15 @@ def generate_add_dataset_html() -> str: if (!emdbEntryName(val("f-name"))) { setError("f-name", "Required"); ok = false; } else setError("f-name", ""); if (!requireField("f-description")) ok = false; - var src = val("f-source"); - if (!src) { setError("f-source", "Required"); ok = false; } - else if (!/^https?:\/\/\S+$/i.test(src)) { setError("f-source", "Must be an http(s) URL"); ok = false; } - else setError("f-source", ""); - var url = val("f-url"); - if (url && !/^https?:\/\/\S+$/i.test(url)) { setError("f-url", "Must be an http(s) URL"); ok = false; } - else setError("f-url", ""); - if (!requireField("f-file")) ok = false; + var url = link(); + if (!url) { setError("f-link", "Required"); ok = false; } + else if (!/^https?:\/\/\S+$/i.test(url)) { setError("f-link", "Must be an http(s) URL"); ok = false; } + else setError("f-link", ""); + // The file name is only asked for when the link does not already end in it. + if (!emdbSplitUrl(url).file && !val("f-file")) { + setError("f-file", "Required - the link does not end in a file name"); + ok = false; + } else setError("f-file", ""); var cs = val("f-checksum"); if (cs && !/^md5:[0-9a-fA-F]{32}$/.test(cs)) { setError("f-checksum", "Must match md5:<32 hex>"); ok = false; } else setError("f-checksum", ""); @@ -1573,6 +1640,50 @@ def generate_add_dataset_html() -> str: ta.remove(); } + // The md5 of a picked file, hashed a chunk at a time so that a multi-GB file + // is never held in memory. `done("")` when the file could not be read. + function hashFile(file, onProgress, done) { + var CHUNK = 8 * 1024 * 1024; + var spark = new window.SparkMD5.ArrayBuffer(); + var reader = new FileReader(); + var offset = 0; + function next() { reader.readAsArrayBuffer(file.slice(offset, offset + CHUNK)); } + reader.onerror = function () { done(""); }; + reader.onload = function (e) { + spark.append(e.target.result); + offset += e.target.result.byteLength; + onProgress(file.size ? offset / file.size : 1); + if (offset < file.size) next(); else done(spark.end()); + }; + next(); + } + + if (localFile) { + localFile.addEventListener("change", function () { + var file = localFile.files && localFile.files[0]; + if (!file) return; + if (!val("f-file")) setValue("f-file", file.name); + setValue("f-size_bytes", String(file.size)); + refresh(); + if (!window.SparkMD5) { + localHint.textContent = "Filled in the name and size of " + file.name + + ". The checksum has to be typed in - the md5 script did not load."; + return; + } + localHint.textContent = "Hashing " + file.name + " - 0%"; + hashFile(file, function (fraction) { + localHint.textContent = "Hashing " + file.name + " - " + + Math.round(fraction * 100) + "%"; + }, function (digest) { + if (digest) setValue("f-checksum", "md5:" + digest); + localHint.textContent = digest + ? "Filled in the name, size and md5 of " + file.name + "." + : "Could not read " + file.name + " - the checksum has to be typed in."; + refresh(); + }); + }); + } + form.addEventListener("input", refresh); form.addEventListener("change", refresh); diff --git a/docs/source/contributing.rst b/docs/source/contributing.rst index 71f2a55..954ec1c 100644 --- a/docs/source/contributing.rst +++ b/docs/source/contributing.rst @@ -12,11 +12,12 @@ Three routes Fill in the `new-dataset issue form `_ and an action turns it into the YAML file and opens a pull request for you. Or -run the CLI below, which writes the file locally and leaves the pull request to -you. Both end in the same place, and both run the same validator. The -:doc:`Add Dataset ` form and the issue form carry every field the -schema has, including model weights and a ``url`` for a download link that is -not ``source/file``. +fill in the :doc:`Add Dataset ` form, which builds the YAML in the +browser and sends you to GitHub with the file pre-filled. Or run the CLI below, +which writes the file locally and leaves the pull request to you. All three end +in the same place, and all three run the same validator. Neither web route has +to be given the checksum or the size: a pull request carrying an entry that is +missing either one has the file downloaded and the fields filled in for it. Techniques ---------- @@ -35,6 +36,28 @@ that is nothing like any of them warns and asks for it to be added to belongs to and in alphabetical order, with ``Other`` staying at the end of ``acquisition``. +Using the web form +------------------ + +The :doc:`Add Dataset ` form carries every field the schema has, +model weights included. It asks for one **Download link**: the direct link to +the file, which it splits into ``source`` and ``file`` the way the CLI does, or +keeps whole as ``url`` when the file is not served at ``source/file``. A Google +Drive share link - the link the share button copies, ``file/d//view`` or +``open?id=`` - is rewritten to the ``uc?export=download&id=`` link that +serves the file. The file name is only asked for when the link does not end in +one. + +The **Local file** picker fills in the file name, size and md5 from the copy on +your machine, so the checksum need not be computed by hand. The file is read in +the browser and nothing is uploaded; a multi-GB file is hashed a chunk at a +time, with progress under the picker. The three fields it fills stay editable. + +**Checksum** and **Size (bytes)** may be left blank, in which case the pull +request downloads the file and fills them in. The picker is the quicker route +for a large file, and a pull request from a fork has to use it, because a fork's +branch cannot be pushed to. + Using the CLI ------------- @@ -42,8 +65,9 @@ Using the CLI python -m emdatabase.new_dataset https://zenodo.org/records/15490547/files/PdNiP.zspy -It splits the URL into ``source`` and ``file``, asks the server for the file's -size, streams the file to a temporary location to compute its md5 (deleted +It splits the URL into ``source`` and ``file`` - a Google Drive share link is +rewritten to its ``uc?export=download&id=`` form first - asks the server for +the file's size, streams the file to a temporary location to compute its md5 (deleted afterwards unless you pass ``--keep``), then prompts for the description, techniques, licence, detector, microscope, voltage, camera length, DOI, tags and authors. Techniques and tags are comma-separated, so a dataset that is both @@ -135,6 +159,14 @@ close to one already on the list fails as a misspelling, while a genuinely new one warns and asks for it to be added. A weights entry without an ML task, and a dataset with one, fail as well. +``fill_download_fields.yml`` runs on every pull request that touches +``emdatabase/index/``. It downloads the file behind each changed entry that is +missing its ``checksum`` or ``size_bytes`` - a weights family's ``latest`` and +each dated version on their own links - fills the fields in and pushes the +result back to the branch, which is how an entry from the web form or the issue +form ends up complete. A fork's branch cannot be pushed to, so a pull request +from one fails instead and prints the values to paste in. + ``check_sources.yml`` runs weekly and asks each source server whether the file is still there and still the size the entry claims. @@ -186,5 +218,6 @@ GitHub archival is only for links that move in place, Google Drive among them. Google Drive works for a small file, as a ``https://drive.google.com/uc?export=download&id=`` link written to the entry's ``url``; above about 100 MB Drive answers with a virus-scan page -instead of the file, and the entry will not download. The CLI recognises a link -like that and fills in ``url``, ``source`` and ``file`` itself. +instead of the file, and the entry will not download. All three routes take the +share link as well and rewrite it to that form, and fill in ``url``, ``source`` +and ``file`` themselves. diff --git a/emdatabase/new_dataset.py b/emdatabase/new_dataset.py index a2a53de..71af40f 100644 --- a/emdatabase/new_dataset.py +++ b/emdatabase/new_dataset.py @@ -5,7 +5,8 @@ prompts for the rest of the metadata and writes ``emdatabase/index/.yaml``. A link that does not end in the file name - a Google Drive link, or anything else with a query string - is written out as -``url``, with the file name taken from the server. Nothing is written until the +``url``, with the file name taken from the server. A Google Drive share link is +rewritten to the ``uc?export=download&id=`` link that serves the file. Nothing is written until the entry passes :func:`~emdatabase.metadata.validate_document`, which is the same check the test suite and the issue-form workflow run. @@ -17,6 +18,11 @@ ``--validate PATH`` runs that check on a file you wrote by hand and does nothing else. + +:func:`fill_download_fields` is the same download from the other end: it takes a +parsed entry that is missing its ``checksum`` or ``size_bytes`` and fills them +in. The issue route and ``.github/workflows/fill_download_fields.yml`` run it +over an entry the forms left blank. """ from __future__ import annotations @@ -136,6 +142,26 @@ def download_md5( return digest.hexdigest(), downloaded, served, content_type +_DRIVE_FILE = re.compile(r"^https?://drive\.google\.com/file/d/([^/?#]+)", re.IGNORECASE) +_DRIVE_OPEN = re.compile( + r"^https?://drive\.google\.com/open\?(?:[^#]*&)?id=([^&#]+)", re.IGNORECASE +) + + +def normalize_url(url: str) -> str: + """A Google Drive share link as its download link; any other link unchanged. + + The link Drive's share button hands out - ``file/d//view`` or + ``open?id=`` - serves the viewer page, not the file. It carries the same + id as ``uc?export=download&id=``, which serves the bytes, so it is + rewritten to that rather than refused. + """ + match = _DRIVE_FILE.match(url) or _DRIVE_OPEN.match(url) + if match is None: + return url + return f"https://drive.google.com/uc?export=download&id={match.group(1)}" + + def split_url(url: str) -> tuple[str, str, str]: """``(source, file, url)`` for a link, with ``url`` empty when unneeded. @@ -143,8 +169,10 @@ def split_url(url: str) -> tuple[str, str, str]: and the name, which is how nearly every entry is written. One with a query string, or with no extension on its last segment, names nothing: it is kept whole as ``url``, ``source`` is the host it points at, and the file name has - to come from the server. + to come from the server. A Google Drive share link is normalised first, so + what is written out is the link that serves the file. """ + url = normalize_url(url) parts = urllib.parse.urlsplit(url) if not (parts.scheme and parts.netloc): return "", "", "" @@ -260,6 +288,59 @@ def as_weights_family(entry: dict[str, Any], date: str) -> dict[str, Any]: } +def _fill_pin(label: str, pin: dict[str, Any], url: str) -> list[str]: + """Download ``url`` for whichever of the two fields ``pin`` is missing.""" + if pin.get("checksum") and pin.get("size_bytes"): + return [] + with tempfile.TemporaryDirectory() as directory: + digest, downloaded, _, content_type = download_md5( + url, Path(directory) / "download", progressbar=False + ) + if content_type.startswith("text/html"): + raise ValueError( + f"{label}: {url} answered with {content_type}, so it served a page rather than the " + "file - a Google Drive viewer page, or a 404 dressed up as HTML. Fix the link." + ) + filled = [] + if not pin.get("checksum"): + pin["checksum"] = f"md5:{digest}" + filled.append(f"checksum {pin['checksum']}") + if not pin.get("size_bytes"): + pin["size_bytes"] = downloaded + filled.append(f"size_bytes {downloaded}") + return [f"{label}: filled in {' and '.join(filled)} from {url}"] + + +def fill_download_fields(document: dict[str, Any]) -> list[str]: + """Fill in every missing ``checksum`` and ``size_bytes`` in a parsed document. + + The docs form and the issue form both let those two fields be blank, and + every entry needs both, so whatever is missing is computed here by + downloading the file. A weights family carries them per pin rather than at + the top level, so ``latest`` and each dated version is followed on its own + link. An entry that already has both is not downloaded. + + The document is filled in place and nothing is written - the caller decides + where the result goes. The lines returned say what was filled, and are empty + when nothing was. + """ + lines: list[str] = [] + for name, entry in document.items(): + if entry.get("kind") == "weights": + pins = [(f"{name} latest", entry.get("latest"))] + pins += [ + (f"{name} version {date}", pin) + for date, pin in (entry.get("versions") or {}).items() + ] + for label, pin in pins: + if pin: + lines += _fill_pin(label, pin, pin.get("url", "")) + else: + url = entry.get("url") or f"{entry.get('source', '')}/{entry.get('file', '')}" + lines += _fill_pin(name, entry, url) + return lines + + class _IndexDumper(yaml.SafeDumper): """PyYAML output in the house style of the hand-written index files. @@ -355,7 +436,7 @@ def main(argv: list[str] | None = None) -> int: if not args.url: parser.error("a URL is required (or --validate PATH)") - url = args.url.rstrip("/") + url = normalize_url(args.url.rstrip("/")) source, filename, link = split_url(url) if not source: print(f"{args.url!r} is not a link to a file") diff --git a/emdatabase/tests/test_fill_download_fields.py b/emdatabase/tests/test_fill_download_fields.py new file mode 100644 index 0000000..945054a --- /dev/null +++ b/emdatabase/tests/test_fill_download_fields.py @@ -0,0 +1,142 @@ +"""Tests for the pull-request script that fills in a checksum and a size. + +``.github/scripts/fill_download_fields.py`` downloads whatever an entry is +missing, so the tests point one at the local HTTP server in ``conftest``. The +index it rewrites is a throwaway directory rather than the shipped one. The +script does not ship in the wheel, so it is loaded from its path the way +``test_check_weights`` loads the weights script. +""" + +import hashlib +import importlib.util +import sys +from pathlib import Path + +import pytest +import yaml + +from emdatabase.new_dataset import FIELD_ORDER + +pytest.importorskip("jsonschema") + +SCRIPT = Path(__file__).resolve().parents[2] / ".github" / "scripts" / "fill_download_fields.py" + +FILE = "MyData.zspy" +CONTENT = b"a small 4D-STEM dataset, allegedly" * 100 +MD5 = f"md5:{hashlib.md5(CONTENT).hexdigest()}" + + +@pytest.fixture(scope="module") +def script(): + """The script under test, imported from its path.""" + if not SCRIPT.exists(): + pytest.skip(f"{SCRIPT} is not in this checkout") + spec = importlib.util.spec_from_file_location("fill_download_fields_under_test", SCRIPT) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +@pytest.fixture +def index(http_server, tmp_path): + """``(served directory, index directory, writer)`` for one entry.""" + base, served = http_server + (served / FILE).write_bytes(CONTENT) + directory = tmp_path / "index" + directory.mkdir() + + def write(**extra): + document = { + "MyData": { + "description": "A 4D-STEM dataset of something.", + "source": base, + "file": FILE, + "license": "CC-BY-4.0", + "technique": ["4D-STEM"], + **extra, + } + } + path = directory / "MyData.yaml" + path.write_text(yaml.safe_dump(document, sort_keys=False), encoding="utf-8") + return path + + return served, directory, write + + +def _run(script, directory, tmp_path, *paths): + summary = tmp_path / "filled.md" + code = script.main(["--index", str(directory), "--summary", str(summary), *paths]) + return code, summary.read_text(encoding="utf-8") + + +def test_a_blank_entry_is_filled_in_and_written(script, index, tmp_path): + _, directory, write = index + path = write() + code, summary = _run(script, directory, tmp_path) + + assert code == 0 + entry = yaml.safe_load(path.read_text(encoding="utf-8"))["MyData"] + assert entry["checksum"] == MD5 + assert entry["size_bytes"] == len(CONTENT) + # Rewritten through build_document, so the filled-in fields land in order. + assert list(entry) == [key for key in FIELD_ORDER if key in entry] + assert MD5 in summary and str(path) in summary + + +def test_a_complete_index_is_not_rewritten(script, index, tmp_path): + _, directory, write = index + path = write(checksum=MD5, size_bytes=len(CONTENT)) + before = path.read_text(encoding="utf-8") + + code, summary = _run(script, directory, tmp_path) + assert code == 0 + assert path.read_text(encoding="utf-8") == before + assert summary.strip() == "Every entry already has its checksum and size." + + +def test_only_the_files_named_are_looked_at(script, index, tmp_path): + """The workflow passes the files a pull request changed.""" + _, directory, write = index + path = write() + other = directory / "Other.yaml" + other.write_text(path.read_text(encoding="utf-8").replace("MyData:", "Other:"), "utf-8") + + code, _ = _run(script, directory, tmp_path, str(path)) + assert code == 0 + assert "checksum" in path.read_text(encoding="utf-8") + assert "checksum" not in other.read_text(encoding="utf-8") + + +def test_a_link_that_serves_a_page_fails_and_writes_nothing(script, index, tmp_path): + served, directory, write = index + (served / "scan.html").write_text("virus scan warning", encoding="utf-8") + path = write(file="scan.html") + before = path.read_text(encoding="utf-8") + + code, summary = _run(script, directory, tmp_path) + assert code == 1 + assert path.read_text(encoding="utf-8") == before + assert "served a page" in summary + + +def test_a_link_that_does_not_resolve_fails(script, index, tmp_path): + _, directory, write = index + path = write(file="NotThere.zspy") + code, summary = _run(script, directory, tmp_path) + assert code == 1 + assert "NotThere.zspy" in summary or "404" in summary + assert "checksum" not in path.read_text(encoding="utf-8") + + +def test_an_entry_that_will_not_validate_is_not_written(script, index, tmp_path): + """The same check the test suite runs, before anything is rewritten.""" + _, directory, write = index + path = write(technique=["Tomograhy"]) + before = path.read_text(encoding="utf-8") + + code, summary = _run(script, directory, tmp_path) + assert code == 1 + assert path.read_text(encoding="utf-8") == before + assert "Tomograhy" in summary diff --git a/emdatabase/tests/test_forms.py b/emdatabase/tests/test_forms.py index 0741e89..10e45f2 100644 --- a/emdatabase/tests/test_forms.py +++ b/emdatabase/tests/test_forms.py @@ -10,11 +10,13 @@ The docs form builds its YAML in the browser, so the check runs the generator function itself under ``node``; the tests skip when node is not installed. The issue-form script lives in ``.github/scripts`` rather than in the package and is -loaded from its path. Nothing here touches the network - the one call that would -(``content_length``) is stubbed out. +loaded from its path. Nothing here touches the network: the calls that would +are either stubbed out (``content_length``) or pointed at the local HTTP server +in ``conftest``. """ import datetime +import hashlib import importlib.util import json import shutil @@ -33,7 +35,13 @@ techniques, validate_document, ) -from emdatabase.new_dataset import FIELD_ORDER, as_weights_family, build_document +from emdatabase.new_dataset import ( + FIELD_ORDER, + as_weights_family, + build_document, + normalize_url, + split_url, +) pytest.importorskip("jsonschema") @@ -96,13 +104,37 @@ def run(fields): return run +@pytest.fixture +def run_split(build_docs, tmp_path): + """Run the form's link splitter under node; return ``(source, file, url)``.""" + node = shutil.which("node") + if node is None: + pytest.skip("node is not installed") + script = tmp_path / "split_url.js" + script.write_text( + build_docs.ADD_DATASET_YAML_JS + + '\nvar p = emdbSplitUrl(require("fs").readFileSync(0, "utf8"));\n' + + "process.stdout.write(JSON.stringify([p.source, p.file, p.url]));\n", + encoding="utf-8", + ) + + def run(url): + result = subprocess.run( + [node, str(script)], input=url, capture_output=True, text=True, check=True + ) + return tuple(json.loads(result.stdout)) + + return run + + VERSION_DATE = "260902" +DRIVE_LINK = "https://drive.google.com/uc?export=download&id=1inQ6DQ2zH40CcdTSiXGnpG" + DATASET_FIELDS: dict[str, Any] = { "name": "MgONanoCrystals", "description": "A 4D-STEM dataset of MgO nanocrystals, calibrated in mrad.", - "source": "https://drive.google.com", - "url": "https://drive.google.com/uc?export=download&id=1inQ6DQ2zH40CcdTSiXGnpG", + "link": DRIVE_LINK, "checksum": "md5:df9376d5c020a23f0f7f51cfe79f303f", "file": "MgONanoCrystals.zspy", "size_bytes": "1104287335", @@ -167,7 +199,8 @@ def test_form_dataset_with_an_opaque_url_validates(run_form): document, entry = _entry(text) assert validate_document(document) == [] _assert_in_field_order(entry) - assert entry["url"] == DATASET_FIELDS["url"] + assert entry["source"] == "https://drive.google.com" + assert entry["url"] == DRIVE_LINK assert entry["file"] == "MgONanoCrystals.zspy" assert entry["size_bytes"] == 1104287335 assert entry["authors"] == { @@ -197,7 +230,7 @@ def test_form_weights_validates_and_carries_the_model(run_form): "framework": "torch", "quantem": ">=0.2,<0.3", } - _assert_weights_family(entry, WEIGHTS_FIELDS["url"], int(WEIGHTS_FIELDS["size_bytes"])) + _assert_weights_family(entry, DRIVE_LINK, int(WEIGHTS_FIELDS["size_bytes"])) def test_form_takes_today_when_the_version_date_is_blank(run_form): @@ -214,15 +247,73 @@ def test_form_drops_the_model_block_for_a_dataset(run_form): def test_form_omits_the_url_when_the_file_is_at_source_slash_file(run_form): - fields = dict(DATASET_FIELDS, source="https://zenodo.org/records/15490547/files", url="") - _, entry = _entry(run_form(fields)) + link = "https://zenodo.org/records/15490547/files/MgONanoCrystals.zspy" + _, entry = _entry(run_form(dict(DATASET_FIELDS, link=link, file=""))) assert "url" not in entry + assert entry["source"] == "https://zenodo.org/records/15490547/files" + assert entry["file"] == "MgONanoCrystals.zspy" + + +# Every shape of link the form may be handed, run through both implementations. +LINKS = ( + "https://drive.google.com/file/d/1jHE-XImhTFI9sFVdyvUWfwOXhdsxvPQV/view?usp=drive_link", + "https://drive.google.com/file/d/1jHE-XImhTFI9sFVdyvUWfwOXhdsxvPQV/view", + "https://drive.google.com/file/d/1jHE-XImhTFI9sFVdyvUWfwOXhdsxvPQV/edit", + "https://drive.google.com/open?id=1jHE-XImhTFI9sFVdyvUWfwOXhdsxvPQV", + "https://drive.google.com/open?usp=drive_link&id=1jHE-XImhTFI9sFVdyvUWfwOXhdsxvPQV", + DRIVE_LINK, + "https://zenodo.org/records/15490547/files/smallPtychography.hspy", + "https://zenodo.org/records/15490547/files/smallPtychography.hspy?download=1", + "https://github.com/electronmicroscopy/emdatabase/raw/abc1234/data/small.zspy", + "https://example.com/downloads/no-extension-here", + "https://example.com", + "HTTPS://Example.COM/a/b.zspy", + "not a url", +) + + +@pytest.mark.parametrize("url", LINKS) +def test_form_splits_a_link_the_way_the_cli_does(run_split, url): + """One field, two implementations: the JS port has to answer as split_url does.""" + assert run_split(url) == split_url(url) + + +def test_form_takes_a_drive_share_link(run_form): + share = "https://drive.google.com/file/d/1jHE-XImhTFI9sFVdyvUWfwOXhdsxvPQV/view?usp=drive_link" + fields = dict(DATASET_FIELDS, link=share, file="example.zspy") + document, entry = _entry(run_form(fields)) + assert validate_document(document) == [] + assert entry["source"] == "https://drive.google.com" + assert entry["url"] == normalize_url(share) + assert entry["file"] == "example.zspy" + + +def test_form_offers_a_local_file_picker(build_docs): + """The picker fills in name, size and md5; the md5 comes from SparkMD5.""" + html = build_docs.generate_add_dataset_html() + assert 'id="f-localfile" type="file"' in html + assert 'id="hint-f-localfile"' in html + assert "spark-md5/3.0.2/spark-md5.min.js" in html + + +def _field_html(html, fid): + """The one field block for ``fid``: from its label to its error line.""" + return html[html.index(f'