Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update validator.yml to fix maintenance issues (dependencies, disk space, error throwing) #168

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
44 changes: 44 additions & 0 deletions .github/workflows/check-line-endings.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@

name: Check for Windows line endings
on:
push:
branches: [ master ]
pull_request:
branches: [ master ]

jobs:
check-line-endings:
runs-on: ubuntu-latest

steps:

- name: Checkout
uses: actions/checkout@v2
with:
# make sure to download directly from the PR's repo, whether that is this repo or a fork
# By default github generates a merge commit for each PR in this repo, but only for the one branch under test
# but `git-annex` needs access to *two* branches: the current branch and `git-annex`
# this might be subtly buggy since it is testing the remote version, not the merged version
#
# *if* this is not a pull request, this will fall back to its default behaviour.
repository: ${{github.event.pull_request.head.repo.full_name}}
ref: ${{ github.event.pull_request.head.ref }}

- name: Install dos2unix
run: |
# the easy way to do this: convert everything to unix and ask git if that changed anything
sudo apt-get install -y dos2unix

- name: Check line endings
run: |
# the easy way to do this: convert everything to unix and ask git if that changed anything

find ./ -path ./.git -prune -o -type f -print0 | xargs -0 dos2unix -q
# this version is safer, but more maintenance:
#find ./ -path ./.git -prune -o -type f \( ! -name "*.nii" -a ! -name "*.nii.gz" \) -print0 | xargs -0 dos2unix -q

git diff --stat --exit-code
if [ "$?" -ne 0 ]; then
echo "error: Windows line endings found."
exit 1
fi
28 changes: 28 additions & 0 deletions .github/workflows/lint-participants.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@

name: Lint participants.tsv file
on:
push:
branches: [ master ]
pull_request:
branches: [ master ]

jobs:
lint-participants:
runs-on: ubuntu-latest

steps:

- name: Checkout
uses: actions/checkout@v2
with:
# make sure to download directly from the PR's repo, whether that is this repo or a fork
# By default github generates a merge commit for each PR in this repo, but only for the one branch under test
# but `git-annex` needs access to *two* branches: the current branch and `git-annex`
# this might be subtly buggy since it is testing the remote version, not the merged version
#
# *if* this is not a pull request, this will fall back to its default behaviour.
repository: ${{github.event.pull_request.head.repo.full_name}}
ref: ${{ github.event.pull_request.head.ref }}

- name: Lint participants.tsv
run: .github/workflows/lint-tsv participants.tsv
90 changes: 43 additions & 47 deletions .github/workflows/validator.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,18 @@ jobs:

steps:

- name: Gain disk space by remapping temp disk (`/dev/sdb1`)
uses: easimon/maximize-build-space@master
# We use the above action instead of removing unwanted packages because it only takes about 2s;
# by comparison, removing lots of small file is very time-consuming.
with:
# After freeing up space we could have as much as 87GB available, which is overkill.
# So, we set root == 10GB to save room for installing dependencies (leaving us with ~77GB)
root-reserve-mb: 10240
swap-size-mb: 1024

- name: Checkout
uses: actions/checkout@v2
uses: actions/checkout@v4
with:
# make sure to download directly from the PR's repo, whether that is this repo or a fork
# By default github generates a merge commit for each PR in this repo, but only for the one branch under test
Expand All @@ -24,63 +34,33 @@ jobs:
repository: ${{github.event.pull_request.head.repo.full_name}}
ref: ${{ github.event.pull_request.head.ref }}

- name: Install dos2unix
run: |
# the easy way to do this: convert everything to unix and ask git if that changed anything
sudo apt-get install -y dos2unix

- name: Check line endings
- name: Update apt packages
run: |
# the easy way to do this: convert everything to unix and ask git if that changed anything

find ./ -path ./.git -prune -o -type f -print0 | xargs -0 dos2unix -q
# this version is safer, but more maintenance:
#find ./ -path ./.git -prune -o -type f \( ! -name "*.nii" -a ! -name "*.nii.gz" \) -print0 | xargs -0 dos2unix -q

git diff --stat --exit-code
if [ "$?" -ne 0 ]; then
echo "error: Windows line endings found."
exit 1
fi

- name: Lint participants.tsv
run: .github/workflows/lint-tsv participants.tsv

#- name: Update software
# run: |
# # do we want to do this? it's helpful to avoid testing against surprise out-of-date software, but also so slow.
# sudo apt-get update &&
# sudo DEBIAN_FRONTEND=noninteractive apt-get -y dist-upgrade &&
# sudo DEBIAN_FRONTEND=noninteractive apt-get autoremove
# do we want to do this? it's helpful to avoid testing against surprise out-of-date software, but also so slow.
sudo apt-get update &&
sudo DEBIAN_FRONTEND=noninteractive apt-get -y dist-upgrade &&
sudo DEBIAN_FRONTEND=noninteractive apt-get autoremove

- name: Install git-annex
run: |
sudo apt-get install -y git-annex
git config --global annex.thin true
git config --global annex.retry 2

- name: Setup NodeJS (for bids-validator)
uses: actions/setup-node@v4

- name: Install bids-validator
run: |
# install proper NodeJS for bids-validator
curl -sL https://deb.nodesource.com/setup_current.x | sudo -E bash -
sudo apt-get install -y nodejs
sudo npm install -g bids-validator
run: sudo npm install -g bids-validator

- name: Set up Python
uses: actions/setup-python@v2
uses: actions/setup-python@v5
with:
python-version: 3.7
python-version: 3.11

- name: Install spine-generic for acquisition parameters check
run: pip install spinegeneric@git+https:/spine-generic/spine-generic.git@master

#- name: Increase free space
# run: |
# # this takes about 2 minutes but saves about 30GB, which is space we might need for git-annex.
# # annex.thin saves a lot of space, but if the dataset grows beyond what Github can handle
# # try enabling this.
# # ref: https:/actions/virtual-environments/issues/2606#issuecomment-772683150
# sudo rm -rf /usr/local/lib/android /usr/share/dotnet

- name: git config
run: |
# this is needed by git-annex so that it can write to the local git-annex branch
Expand All @@ -93,13 +73,29 @@ jobs:

- name: Download dataset
run: |
git fetch --depth=1 origin git-annex:git-annex # actions/checkout@v2 does a shallow checkout, so it is missing this important branch
git fetch --depth=1 origin git-annex:git-annex # actions/checkout@v4 does a shallow checkout, so it is missing this important branch
git annex init
git annex get -J8

- name: Checking BIDS compliance
run: bids-validator --verbose ./
if: always()
run: |
bids-validator --verbose ./ | tee bids.txt
printf "\nShort summary:\n\n"
# FIXME: This should be replaced with bids-validator settings: A) throw error on warning or B) warning ignores
! grep "\[WARN\]\|\[ERR\]" bids.txt # ! -> Reverse error code so "found == error thrown"

- name: Checking acquisition parameters
run: sg_params_checker -path-in ./
if: always()
run: |
sg_params_checker -path-in ./ | tee params.txt
# FIXME: Checking the output like this should be replaced with SG throwing actual errors
! grep "WARNING" params.txt # ! -> Reverse error code so "found == error thrown"

- name: Checking data consistency
run: sg_check_data_consistency -path-in ./
if: always()
run: |
sg_check_data_consistency -path-in ./ | tee consistency.txt
# FIXME: Checking the output like this should be replaced with SG throwing actual errors
! grep "Warning\|Missing" consistency.txt
grep "all good" consistency.txt
Comment on lines 80 to +101
Copy link
Member Author

@joshuacwnewton joshuacwnewton Jun 27, 2024

Choose a reason for hiding this comment

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

Note: The failures on this PR are to be expected, because all 3 of these checks throw warnings/errors:

  • BIDS validation: Link
    • ERR: code 1 - NOT_INCLUDED
    • WARNING: code 38 - INCONSISTENT_SUBJECTS
    • WARNING: code 39 - INCONSISTENT_PARAMETERS
    • WARNING: code 101 - README_FILE_MISSING
  • Acquisition parameters: Link
    • WARNING: Incorrect RepetitionTime
    • WARNING: Incorrect FlipAngle
    • WARNING: Model 'MAGNETOM Vida' not present
    • WARNING: Missing 'Manufacturer' key in json sidecar
  • Data consistency: Link
    • WARNING: Missing jsonSidecar
    • WARNING: Invalid number of columns. The schema specifies 13, but the data frame has 15

We should brainstorm on how to best fix these issues! Or, if they don't need to be fixed, we should explicitly add ignore rules for them so that they don't get thrown in the first place.

Copy link
Member Author

@joshuacwnewton joshuacwnewton Jun 27, 2024

Choose a reason for hiding this comment

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

BIDS: The WARNINGs can be ignored. (README can be fixed.) ERR should be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Automated testing was done right before a conference, just for demo purposes, so these warnings aren't actually taken into account for acquisition parameters/data consistency.

Loading