Run bids-validator #5
Conversation
|
Some issues:
Also I haven't actually tested plugging it into Gitea properly yet: my test so far is just |
|
I've tried out the worker with Gitea:
Then I pushed to a repo I already had. The extremely appealing part of this design: I didn't have to log into bids-hook, I didn't have to "activate" (i.e. install webhooks) each repo, I didn't have to write in a script (like .drone.yml) to do everything. It Just Worked:
It hung for a while. I inspected and could see it was lagging at running When it finally got to
clicking the red X showed me why:
🎉 |
|
Deployment is more complicated than I would like still, but it's not really more complicated than (though some of that will need to stay: generating the |
Subdirs are simply named by taking the prefix of the input UUID. This is a common technique to prevent the chance of overflowing filesystem limits on the number of files allowed in a single directory. e.g. Before - /srv/gitea/custom/public/90fa9a89-5e50-4bd4-834d-d42655e1ee8e.html - http://127.0.0.1:3000/assets/90fa9a89-5e50-4bd4-834d-d42655e1ee8e.html After: - /srv/gitea/custom/public/bids-validator/84/b1/84b10e1c-b188-41e2-a92d-fb6ded9c6889.html - http://127.0.0.1:3000/assets/bids-validator/84/b1/84b10e1c-b188-41e2-a92d-fb6ded9c6889.html
kousu
force-pushed
the
output-subdirs
branch
from
0598331 to
adba766
Compare
To test: - set up a gitea install at ../gitea/ and run `./start` (if elsewhere, run `GITEA_APP_PATH=path/to/gitea ./start`) - make a repo in it; upload some git-annex (or not git-annex) files - commit and push to the test repo
Similar to results, see 5d3f078. * While we're at it, create the log folder if need be, instead of erroring out if it doesn't exist already. * As a result, we no longer need to import io/fs. * Uniformize permissions to 0750 and 0640 too.
* I added documentation for the script's contract/API in the script itself, for people who want to modify it. * I restored the ERR trap, but managed to still save an exit status using the following idiom: ```sh some_command && STATUS=$? || STATUS=$? ``` * I added a temp dir cleanup trap on EXIT, and checked that the commands in it don't cause ERR traps, and don't affect the script's exit status. * I removed the $BH_UUID from the temp dir name, because that's mildly sensitive info, and the temp dir name is world-readable. * I removed `--verbose` from `bids-validator .` because it can generate so many lines that it's hard to find the actual warning types in the output. * With an extra dependency on `jq`, I added a way to check whether `bids-validator` returns success with or without warnings. * I added an `ansifilter` step to turn the terminal color escape sequences and characters like `<`, `&` into valid HTML. (I guess we should similarly sanitize $BH_USER, etc.?) * I made the script exit status conform to its contract/API.
Hopefully this name is more inviting for people who want to mod it.
|
Thanks for researching the best steps to use for the git checkout! I revamped the worker script, and tested it in the following scenarios:
After removing a bunch of the bugs I put in, I got it to show me yellow dots, green checkmarks, red exes, yellow exclamations, and red exclamations, along with results pages and log files. One question is: do we want to bother with showing warnings as yellow marks? If so, maybe we should supply an example Another question is, do we want to configure the webhook to ignore the branches |
|
(@kousu I can't add you as a reviewer because you're the PR author, but I would love to hear your comments.) |
|
Ah wonderful. Thanks for trying out all those cases. I don't suppose you kept screenshots of each case? I'd like to look over the log files. Maybe do you have a repo with branches for each of the cases you tested I could pull somewhere?
Yes, definitely, that's super handy. I guess you're worried that it will train people to ignore it though? I'm unhappy that BIDS is sort of 99 standards in one so that it needs to be the kind of standard where people often ignore the warnings they don't care about. Ideally our datasets conform the baseline BIDS, and we fix the data not the warnings. But either way including
mmmmmyesssss but I think it'd be better to solve this in gitea itself: neuropoly/gitea#4. I wish this script didn't have to know about git-annex.
I'll look over the worker and give thoughts now! |
| OUTPUT=$(bids-validator .) && STATUS=$? || STATUS=$? | ||
| WARNINGS=$(bids-validator . --json | jq '.issues.warnings | length') || true | ||
| cat 1>&2 <<EOF | ||
| STATUS=${STATUS} | ||
| WARNINGS=${WARNINGS} | ||
| EOF | ||
|
|
||
| echo 1>&2 '# formatting output' | ||
| HTML=$(echo "$OUTPUT" | ansifilter --html --fragment --line-numbers --anchors=self) | ||
|
|
||
| # Produce the HTML result page on stdout. | ||
| cat <<EOF | ||
| <!DOCTYPE html> | ||
| <html lang=en> | ||
| <title>bids-validator results for ${BH_USER}/${BH_REPO}@${BH_COMMIT}</title> | ||
| <p>Here are the bids-validator results for ${BH_USER}/${BH_REPO}@${BH_COMMIT}:</p> | ||
| <pre> | ||
| ${HTML} | ||
| </pre> | ||
| </html> | ||
| EOF |
There was a problem hiding this comment.
This buffers the output and runs it through the shell. Is echo "$OUTPUT" safe? What happens if there's nuls in the output? Weird unicode? What happens if the data is huge and overflows RAM? It's a lot safer to pipe:
cat <<EOF
<!DOCTYPE html>
<html lang=en>
<title>bids-validator results for ${BH_USER}/${BH_REPO}@${BH_COMMIT}</title>
<p>Here are the bids-validator results for ${BH_USER}/${BH_REPO}@${BH_COMMIT}:</p>
<pre>
EOF
(bids-validator . | ansifilter --html --fragment --line-numbers --anchors=self) && STATUS=$? || STATUS=$?
cat <<EOF
</pre>
</html>
EOF
(pipes will block if they're out of space until they're not out of space)
That was why I was using nested subshells -- you can grab their exit codes while still processing data 'live', piped direct to stdout.
There was a problem hiding this comment.
TIL that bash ignores null bytes when doing variable expansions. At least it prints out a warning that this is the case! I thought with enough quoting it would be safe (apart from the issue that trailing newlines are discarded when doing $(...)), but I was insufficiently paranoid. This is a good lesson to learn, thanks.
But since pipes are safe, let's use them, yes. Maybe wrapped in a function, if we want to be nice about formatting the script.
| EOF | ||
|
|
||
| ( | ||
| WORKDIR="$(mktemp -d --tmpdir bids-hook."$BH_UUID".XXXXXXXXXXX)" |
There was a problem hiding this comment.
It doesn't change much either way, but why do you think BH_UUID is sensitive?
There was a problem hiding this comment.
My thinking is that the BH_UUID is the only "secret" which is needed to view someone else's bids-validator output, because Gitea doesn't have any way to authenticate visitors of the static assets. And this output can contain a bunch of metadata about subjects, etc.
I guess we have bigger problems if someone is logged into the server anyway, but just doing ls /tmp at the right time, while logged in as any user, would reveal the BH_UUID for the running jobs, and then you can visit the corresponding webpage with the bids-validator output.
| GITEA_REPO=$(realpath --canonicalize-existing \ | ||
| "${GITEA_REPOSITORY_ROOT}/${BH_USER}/${BH_REPO}.git") |
There was a problem hiding this comment.
Because I wanted to be more transparent in the output, I wanted; which meant showing a relative path to the user, maybe.
I just did a quick test and git clone canonicalizes local paths; but git remote add doesn't. So..yeah I guess we need this either way; i didn't test with GITEA_REPOSITORY_ROOT set to a relative path I guess. And we'll just have to live with exposing full paths. Drone does, after all (but Drone mostly works in ephemeral docker containers)
There was a problem hiding this comment.
I see. I actually ran my tests with relative paths, and everything failed, that's why I put this in. And since I wasn't trying to show this path to users, only in the admin logs, I hadn't thought about showing a pretty path. Good point.
| if git ls-remote --exit-code "$GITEA_REPO" "refs/heads/git-annex" >/dev/null; then | ||
| ANNEXED=1 | ||
| else | ||
| ANNEXED="" | ||
| fi | ||
|
|
||
| if [ -n "$ANNEXED" ]; then | ||
| set -x | ||
| # If this is a git-annex repository, we need to get the contents. | ||
| if git ls-remote --exit-code origin refs/heads/git-annex >/dev/null; then | ||
| echo '# getting git-annexed files' | ||
| # this reduces copies; always overrides annex.hardlink even if that is set system-wide | ||
| git config annex.thin true | ||
| # make sure we don't corrupt origin accidentally | ||
| git config remote.origin.annex-readonly true | ||
| git config annex.private true # XXX this doesn't do anything until git-annex 10 | ||
| git annex init | ||
| git annex dead here # this is like annex.private, but has to be run | ||
| git annex sync --only-annex --no-content # grab the git-annex branch (since we did a shallow clone above) | ||
| git annex copy --from origin # NB: using copy --from origin and not git annex get to ensure we're validating the contents of origin and not any special remotes | ||
| else | ||
| set -x | ||
| # grab the git-annex branch (since we did a shallow clone above) | ||
| git annex sync --only-annex --no-content | ||
| # NB: using 'copy --from origin' and not 'git annex get; to ensure we're | ||
| # validating the contents of origin and not any special remotes | ||
| git annex copy --from origin | ||
| fi |
There was a problem hiding this comment.
I think users should see all these commands.
Drone inserts a clone step into every pipeline by default (unless you turn it off). It appears like this, with each input command being written + CMD (from set +x, or maybe from emulating it) and followed by it's output (stderr and stdout and all):
I think it's easier to understand set -x's output ("+ git remote add ${GITEA_REPO} && git fetch ...") than printed comments ("# cloning ${GITEA_REPO}"). It's longer, but when things go wrong -- which is the main time people read these logs -- people will need to see what commands were actually run. I want that to show up in the final output HTML. Our curators need to be able to have a chance to see what went wrong, without resorting to asking their sysadmin to go dig out the logfile and help them debug git-annex every time. That has no chance of scaling up, especially since I hope eventually we have enough installs and neurogitea is reliable enough that there can be sysadmins who do not need to be intimately familiar with git-annex 🙏
So, I think:
-
set -xis important (which implies2>&1'ing this section, maybe the whole script) -
errors in any of these commands count as exit code 1, not exit code 3
-
I'll grant that 'git: command not found' or 'git-annex: not found', maybe '${GITEA_REPO}: not found', should theoretically be exit code 3 since that's a server misconfiguration. But anything other error the user needs to see. And since it's hard to tease apart which is which, we should just dump all errors here to exit code 2.
A very common error for curators will be some variant of running
git annex copy --tobut forgetting togit annex sync(or worse: force pushing togit-annex) or otherwise gettinggit-annexout of sync with the branch under test is the kind of error that curators need to get feedback on. Arguably we should have a separate webhook to check git-annex health, but even if we did the error should also show up when trying to run this; on other CI systems, you would expect a corrupted version control database to show up as an error when trying to run the tests.
-
There was a problem hiding this comment.
I see now that I was working from a completely different mental model of who should see what, sorry! But your points make a lot of sense.
| if [ -n "$ANNEXED" ]; then | ||
| set -x | ||
| # If this is a git-annex repository, we need to get the contents. | ||
| if git ls-remote --exit-code origin refs/heads/git-annex >/dev/null; then | ||
| echo '# getting git-annexed files' |
There was a problem hiding this comment.
This was an exception to my own rule that everyone should see everything. I originally had it written this way, but with set -x turned on, and it turned out that ls-remote is pretty noisy, whether it succeeds or fails, so I decided users could live without it. It's not something anyone would normally run anyway.
By the way, can you think of a better way to detect if a repo is annexed? git annex init contacts the remote and reads its config, looking for annex.uuid, but I don't know how to do that from the command line without actually running git annex init and by then it's too late.
| cat <<"EOF" | ||
| echo 1>&2 '# running bids-validator' | ||
| OUTPUT=$(bids-validator .) && STATUS=$? || STATUS=$? | ||
| WARNINGS=$(bids-validator . --json | jq '.issues.warnings | length') || true |
There was a problem hiding this comment.
It's disappointing to have to run this twice. bids-validator is pretty quick, even on large datasets, but I don't trust it to stay that way.
Is there another flag where maybe we could get the warnings routed...elsewhere?
Or can we patch bids-validator so that it exits:
- 0 if no issues exist
- 1 if issues exist and contain errors
- 2 if issues exist
Does it already do that?
There was a problem hiding this comment.
Does it already do that?
I don't think so. Looking at the source, here is the meaning of each exit status for bids-validator:
| fi | ||
|
|
||
| bids-validator --verbose . |
There was a problem hiding this comment.
I prefer having verbose on.
I see that we do this in
but not in
So maybe it's a cultural thing.
My argument is that: the CI machine is a batch job. You let it run and wait for it to bake (or boil). If something goes wrong, you don't want to redo its work because that's potentially expensive. So if we're going to output any errors/warnings we should output all of them.
We can help out the cognitive load by colouring errors in red, and warnings in yellow (which ansifilter does now!).
And with bids-validator in particular, not having --verbose makes it quit early when listing issues, which means the most direct route people will try to follow to fix them will be: fix the ones on the screen, commit, push, wait, look at the new list of failures. In that case what people should do, I guess, is see that CI failed, ignore the rest of its output, run bids-validator --verbose locally on their dataset until it passes, then commit and push. But people often will just try to take a shortcut and not do that. And also CI/dev are subtly different environments (for example: if somehow the annexed data is corrupted/out of sync/not committed, dev could pass but CI might fail, and CI is there to catch that kind of mistake).
There was a problem hiding this comment.
I think I'm swayed by your point that bids-validator will quit early without --verbose, which I didn't know.
But for the record, my main reason came from recent experience with --verbose being counter-productive: for example, the output for the "Checking BIDS compliance step" in spine-generic/data-multi-subject is literally more than 10,000 lines. No amount of colour highlighting can make that pleasant. And at least in my browser, Github's UI looks like it's doing some kind of fancy loading and unloading of the content, which breaks text search. If I scroll to the top, I can find the first warning; if I scroll to the bottom, I can find the third warning; but I have not yet been able to find the second warning buried in that mountain of output.
I don't have screenshots, and I'm not sure which log file is which case, sorry. I should get in the habit of taking screenshots. For the repos I basically used spine-generic/data-single-subject, with most of the subjects deleted so that it would be a small example. That was for the git-annexed version, and then I just copied the files (including the real content) to a new repo for the non-git-annex tests. |
| git checkout "$BH_COMMIT" | ||
|
|
||
| # If this is a git-annex repository, we need to get the contents. | ||
| if git ls-remote --exit-code origin refs/heads/git-annex >/dev/null; then |
There was a problem hiding this comment.
Since we know the repository is local anyway, we could instead check directly for a valid annex.uuid in the config:
| if git ls-remote --exit-code origin refs/heads/git-annex >/dev/null; then | |
| if git -C "$GITEA_REPO" config --get annex.uuid '^[[:xdigit:]]{8}(-[[:xdigit:]]{4}){3}-[[:xdigit:]]{12}$' >/dev/null; then |
That would be great, yes. But also here is some silliness that I'm recording for future reference:
None of these suggestions is really serious. |
Fixes #2
Follows #7