ENH: add ability to test examples PR against PR in bids-specification #515

yarikoptic · 2025-09-18T17:43:45Z

Many PRs are developed to accompany WiP BEP PRs. It is beneficial to establish testing against likely a modified BIDS schema in those PRs. In this solution we should expand matrix of runs with bids-pr run against that arbitrary PR if entered in a line with bids-specification-pr:.

TODOs

test with proposed line in the template
BIDS Specification PR: nothing
BIDS Validator PR: NONE
That was original testing -- now will need to redo for above both spec and validator being (not or specified):
- should not run/fail if not a match
- should run correctly if number
- ~~should run correctly if #number (TODO)~~ decided not to bother since would lead on github to local PR etc
- should run correctly if URL
- see what happens if schema is bad (according to https://github.com/bids-standard/bids-schema/tree/main/PRs 2140 is no good ATM): well -- it is kinda different point since there is a .json already, and #2140 was failing to build for us...
- should rerun when description edited
  - it is kinda needed to react when people edit/enter URL but annoying since even checkmarks would trigger it. May be not to do?

effigies · 2025-09-18T18:25:41Z

Please see #480 and #514 for examples of testing BEPs against schema and validator branches.

yarikoptic · 2025-09-18T18:37:42Z

Yep, there temporarily changes the workflow. This is what this tries to avoid and make it more explicit and so it could just be all handled via explicit metadata in description and then merge

effigies · 2025-09-18T18:41:25Z

Sounds good.

effigies

The current process is to prevent non-dev runs from testing new examples, and use validator and schema from releated PRs in dev. Instead of adding a new bids-pr run, I think it would be helpful just to automate the changes to dev.

If you want the run name to be clearly different from dev, then I would replace dev with the other name in the matrix, but have the new name use identical steps to dev.

As to triggers, I don't really like the false positive rate of these triggers. I wonder if we could use a label trigger. Maybe the job could remove the trigger, but we could at least remove and re-add the label to re-trigger.

WDYT?

.github/pull_request_template.md

.github/workflows/validate_datasets.yml

effigies · 2025-09-19T15:58:50Z

.github/workflows/validate_datasets.yml

+        if [ -n "${{ steps.find-pr.outputs.pr_number }}" ]; then
+          EXTRA_ITEM=', "bids-pr"'
+        fi
+        echo "matrix=[\"stable\", \"main\", \"dev\", \"legacy\"${EXTRA_ITEM}]" >> $GITHUB_OUTPUT


I would either deactivate dev or just update dev steps to use your environment variables.

why to collide the two making it to change meaning of a CI run? "explicit better than implicit", so I would have just added one dedicated run so if dev passes somehow -- should alert people if they expect non-released bids-specification to fail

I called it dev-prs now to reflect the fact that we still use dev version of validator even if no PR is given for it.

effigies · 2025-09-19T16:03:06Z

.github/workflows/validate_datasets.yml


    - name: Install BIDS validator (main)
-      if: matrix.bids-validator == 'main'
+      if: matrix.bids-validator == 'main' || matrix.bids-validator == 'bids-pr'


In general, dev tracks unreleased changes to the schema, while main tracks the latest released version of BIDS. PRs against the spec will mostly aim for dev.

yes, but it is useful to know if it fails against released version since if it doesn't but expects to (e.g. new file type was introduced like atlas-<label>_description.json but released validator+schema is ok with that means that either they miss something or something else is not stringent enough. The same about dev as -- have we made development validator and/or schema less stringent, as we would expect then only the dev-pr to pass in such cases. That is why I preferred to keep it separate and additional matrix run.

…ecification-pr

Allow validate_datasets workflow to re-run when PR description is edited, enabling dynamic detection of bids-specification-pr references added after PR creation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Fix missing closing bracket in matrix JSON output - Fix variable name mismatch (PR_BODY_NUM vs PR_NUM) - Fix regex pattern for PR detection (escape sequences) - Fix bash conditional syntax (use || instead of 'or') 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

Use heredoc to safely handle PR body content that may contain: - Quotes (single and double) - Newlines and multiline text - Special bash characters 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

Co-authored-by: Chris Markiewicz <[email protected]>

yarikoptic · 2025-12-15T18:59:37Z

If you want the run name to be clearly different from dev, then I would replace dev with the other name in the matrix, but have the new name use identical steps to dev.

recommendations? could be master or main I guess to reflect that it is a branch

.github/workflows/validate_datasets.yml

…to use SPEC_PR instead of BIDS_PR var Also renamed into `dev-prs` to ensure reflecting the fact that we are installing a development version of the validator even if PR for it not given.

yarikoptic · 2025-12-16T20:13:20Z

eh, OSX keeps giving grief -- even Windows works!

  # Fetch PR info (fork URL and branch) - avoid mapfile for macOS compatibility
  pr_json=$(curl -s https://api.github.com/repos/bids-standard/bids-validator/pulls/305)
  fork_url=$(echo "$pr_json" | jq -r '.head.repo.html_url')
  branch=$(echo "$pr_json" | jq -r '.head.ref')
  echo "I: Received fork_url=$fork_url branch=$branch"
  # Perform the same installation as in "dev"
  git clone -b "$branch" "$fork_url" ../bids-validator
  cd ../bids-validator
  deno compile -A -o $HOME/.deno/bin/bids-validator src/bids-validator.ts
  shell: /bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    SPEC_PR: 2231
    VAL_PR: 305
    TZ: Europe/Berlin
    FORCE_COLOR: 1
I: Received fork_url=null branch=null
fatal: repository 'null' does not exist

Switch from curl to gh api for fetching PR info. The gh cli handles authentication automatically and avoids GitHub API rate limits that were causing failures on macOS runners (60 req/hr unauthenticated vs 1000 req/hr authenticated). Also add error checking for failed API responses. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

yarikoptic · 2025-12-16T21:36:15Z

@effigies I think it works now!!! Please re-review and try on some PRs of interest to you. Would be great to see it in action for PRs like

[BEP028] provenance - SPM preprocessing #497
which require
BEP028 (Provenance) patches bids-validator#305
?

yarikoptic added enhancement bids-validator requires interaction with bids-validator github_actions Pull requests that update GitHub Actions code labels Sep 18, 2025

yarikoptic marked this pull request as ready for review September 18, 2025 20:37

yarikoptic requested a review from effigies September 18, 2025 20:38

yarikoptic mentioned this pull request Sep 18, 2025

feat: Report schema source in validation output bids-standard/bids-validator#269

Draft

1 task

effigies reviewed Sep 19, 2025

View reviewed changes

yarikoptic and others added 6 commits December 15, 2025 13:05

ENH: allow to test against bids-spec PR schema.

8bb8fc8

Add rudimentary pull_requests_template.md with syntax to list bids-sp…

8fa6f5d

…ecification-pr

noop if no match

539efa1

yarikoptic force-pushed the enh-bids-pr-test branch from 6f613fc to 539efa1 Compare December 15, 2025 18:06

yarikoptic and others added 2 commits December 15, 2025 13:07

default to NONE for PRs and add validator PR

0777172

Co-authored-by: Chris Markiewicz <[email protected]>

Adjust for the rename of the tag in prior PR + rename run to bids-prs

a76de45

yarikoptic commented Dec 15, 2025

View reviewed changes

.github/workflows/validate_datasets.yml Outdated Show resolved Hide resolved

yarikoptic force-pushed the enh-bids-pr-test branch 6 times, most recently from ae0834e to 156674b Compare December 16, 2025 14:24

yarikoptic commented Dec 16, 2025

View reviewed changes

.github/workflows/validate_datasets.yml Outdated Show resolved Hide resolved

Install if needed custom bids-validator and also adjust installation …

de7e985

…to use SPEC_PR instead of BIDS_PR var Also renamed into `dev-prs` to ensure reflecting the fact that we are installing a development version of the validator even if PR for it not given.

yarikoptic force-pushed the enh-bids-pr-test branch from 156674b to de7e985 Compare December 16, 2025 14:37

yarikoptic requested review from effigies and rwblair December 16, 2025 21:36

ENH: add ability to test examples PR against PR in bids-specification #515

Are you sure you want to change the base?

ENH: add ability to test examples PR against PR in bids-specification #515

Uh oh!

Conversation

yarikoptic commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Sep 18, 2025

Uh oh!

yarikoptic commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Sep 18, 2025

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

effigies Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

yarikoptic Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

yarikoptic Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

effigies Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

yarikoptic Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

yarikoptic commented Dec 15, 2025

Uh oh!

Uh oh!

Uh oh!

yarikoptic commented Dec 16, 2025

Uh oh!

yarikoptic commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yarikoptic commented Sep 18, 2025 •

edited

Loading

yarikoptic commented Sep 18, 2025 •

edited

Loading