allow full BIDS-acceptable chars as derivative names - #394
Open
asmacdo wants to merge 4 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 79.21% 79.29% +0.07%
==========================================
Files 17 17
Lines 2170 2178 +8
Branches 385 385
==========================================
+ Hits 1719 1727 +8
Misses 308 308
Partials 143 143 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
14 tasks
asmacdo
changed the title
asmacdo
marked this pull request as ready for review
July 23, 2026 15:40
…P job var A zipped input dataset's located zip path is held in a job shell variable named <name>_ZIP, built by uppercasing the dataset name. A name with characters invalid in a POSIX shell identifier (-, ., +, ...) yields an invalid variable name (e.g. SIMBIDS-0.0.3_ZIP) that bash rejects, crashing the job — and BIDS derivative naming (<Tool>-<Version>, e.g. fMRIPrep-24.1.1) makes such names the norm for a derivative used as an input dataset. Add var_safe_name() (non-word chars -> _, uppercased), exposed as the shell_safe_name field on each input's as_dict; the job templates build <shell_safe_name>_ZIP from it. Only the variable name changes; the zip is still located by the real dataset name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… field
The prior commit exposed the shell-safe name as an as_dict field, but babs's tests
(and any caller) build input-dataset dicts directly without it, so the templates
raised jinja2 UndefinedError('shell_safe_name'). Move var_safe_name to utils and
register it as the 'shell_safe' Jinja filter on the job-script environments;
templates now compute '<name | shell_safe>_ZIP' from 'name', which every dict has.
Verified: tests/test_generate_submit_script.py + tests/test_generate_bidsapp_runscript.py
pass in slurm-docker-ci (39 passed). No change to the generated scripts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Derivative names can contain regex metacharacters (the '+' and '.' in e.g. 'fMRIPrep-25.2.5+anat'). The previous single `grep -E` interpolated the name raw into an extended-regex pattern, so '+' was read as a quantifier and never matched the literal '+' in the zip filename — failing every chained job whose upstream derivative name carried a '+'. Match each identifier (subid, sesid, name) as a fixed string via `grep -F`, keeping only the genuine `\.zip$` anchor as a regex. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Renders the zip-locator template and runs the generated finder against a real git tree, asserting the '+'/'.'/etc.-named zip is located. Parametrized over BIDS-plausible metachar names (single '+', double '+', dots) x subject/session. This exercises the coverage gap that let the grep bug through: the chained e2e used producer names without a '+'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
asmacdo
force-pushed
the
sanitize-zip-var
branch
from
July 24, 2026 13:42
860ce3d to
e53a172
Compare
Collaborator
Author
|
Rebased onto current Note: the failing |
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
3 participants
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.
What / why
BIDS derivative naming produces
<Tool>-<Version>names (fMRIPrep-25.2.5+anat,SimBIDS-0.0.3) full of characters —-,.,+— that break the generated job scripts in two places when such a derivative is used as a zipped input dataset. Both fail the running job (notbabs init), and both make BIDS-normal names unusable as chained inputs. This fixes both so any BIDS-valid name works.Changes
<NAME>_ZIP, built by uppercasing the dataset name.SIMBIDS-0.0.3_ZIPis not a valid bash identifier, so the assignment crashes the job. Newvar_safe_name()(inutils.py) maps every non-word char to_and uppercases (SIMBIDS_0_0_3_ZIP); it's registered as theshell_safeJinja filter and applied everywhere the templates build<NAME>_ZIP. A plain name's variable is unchanged.find_single_zip_in_git_treeinterpolated the name into a singlegrep -E, so+/.in the name were read as regex metacharacters and never matched the literal.zipfilename — failing every chained job whose upstream derivative name carried a+. Each identifier (subid, sesid, name) is now matched as a fixed string withgrep -F, keeping only\.zip$as a genuine regex anchor.Verification
tests/test_generate_submit_script.py) renders the zip-locator template and runs the generated finder against a real git tree, parametrized over BIDS-plausible metachar names (single/double+, dots) × subject/session — the exact coverage gap that let the grep bug through.fMRIPrep+anat→fMRIPrep+minimalchaining on the Unity cluster, and confirmed this branch fixes them.The two bugs are independent but share a root cause (metachar-bearing names) and touch the same templates, so they land together.