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

FIX: Repair FreeSurfer Dependency in Dockerfile (tcsh) #404

Merged
merged 4 commits into from Dec 7, 2023

Conversation

psadil
Copy link
Contributor

@psadil psadil commented Nov 30, 2023

Closes #403

FreeSurfer's recon-all requires tcsh, but that shell was no longer being added in the Dockerfile. This installs it.

The lack of tcsh escaped testing during CI because recon-all had been skipped entirely (generally too expensive to run). This change also removes stats/wmparc.stats from the FreeSurfer derivatives for ds005, which ensures that at least some of the recon-all scripts are run (any dependencies that are only used in earlier stages would still be missed).

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3b0dcdb) 76.88% compared to head (13cacd8) 76.98%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
+ Coverage   76.88%   76.98%   +0.09%     
==========================================
  Files          28       28              
  Lines        2042     2042              
  Branches      236      240       +4     
==========================================
+ Hits         1570     1572       +2     
+ Misses        441      438       -3     
- Partials       31       32       +1     
Flag Coverage Δ
ds005 ∅ <ø> (?)
ds054 45.97% <ø> (ø)
pytest 66.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@psadil
Copy link
Contributor Author

psadil commented Nov 30, 2023

Not sure what to make of that failure on ds005. autorecon3 was looking for a file that wasn't present, surf/autodet.gw.stats.lh.dat. FWIW, that is a file that is generated with this version when FreeSurfer derivatives are not already precomputed (and so generated by sMRIPrep).

@psadil
Copy link
Contributor Author

psadil commented Nov 30, 2023

Seems like the autodet.gw.stats.[lr]h.dat files are generated during autorecon2 but are missing from the ReconAllDevTable. They don't seem to be referenced in the nipype freesurfer interface: https://github.com/nipy/nipype/blob/b066807402ee07d908ce4f1b24ca69fef6b91809/nipype/interfaces/freesurfer/preprocess.py#L1387, which would explain why they weren't generated during this CI run.

@effigies
Copy link
Member

Seems so. Maybe we can pick a different thing to delete. Parcstats?

@psadil
Copy link
Contributor Author

psadil commented Nov 30, 2023

Sure, I'll test that locally and report back whether it works

@psadil
Copy link
Contributor Author

psadil commented Nov 30, 2023

Wow, sorry, same basic error (now with lh.orig.premesh). I was trying with a participant that I had locally, but clearly I should be experimenting with ds005

@effigies
Copy link
Member

We should probably just update the prerun FreeSurfer with a fresh run. I've been really procrastinating on that since the right way to do it would be to set up something that is just punch and go and I haven't thought much about the best way to do that.

@psadil
Copy link
Contributor Author

psadil commented Nov 30, 2023

A fresh run does sound cleaner. From what I can tell,

@effigies
Copy link
Member

effigies commented Dec 7, 2023

Spent too much time this week trying to create a datalad dataset that we could keep updated with each version of FreeSurfer. Guess we'll need to keep kicking the can. Thanks for this!

@effigies effigies merged commit 07e5327 into nipreps:master Dec 7, 2023
17 checks passed
@psadil psadil deleted the fix/missing-fs-deps branch December 7, 2023 18:05
effigies pushed a commit that referenced this pull request Dec 8, 2023
Backport of gh-404

Signed-off-by: Christopher J. Markiewicz <markiewicz@stanford.edu>
effigies added a commit that referenced this pull request Mar 20, 2024
0.14.0 (March 11, 2024)

New feature release in the 0.14.x series.

This release restores correct handling of cohort identifiers in templates.
A feature release is warranted due to changes in the workflow structure.

* FIX: Fetch templates during workflow construction (#418)
* FIX: Re-add cohort identifier to template name (#416)
* FIX: Repair FreeSurfer Dependency in Dockerfile (tcsh) (#404)
* FIX: Invert result of skull-strip check in auto mode (#402)
* STY: Adopt ruff for linting and formatting (#397)
* CHORE: Update ruff, ignore certain rules (#419)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

recon-all not detected (v0.13.1) during run with singularity
2 participants