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: Remove unused ANTs parameter that was removed in 2.4.1 #431

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

lalalavi
Copy link
Contributor

@lalalavi lalalavi commented Feb 2, 2024

Hello! :)

Starting with ANTS 2.4.1 there is a flag that has disappeared in the AntsRegistration command and we would like to backport the commit that fixes it ( link ) into this version of sdcworkflows, since Halfpipe depends on it and we want to bump ANTS to solve problems with the FixN4BiasFieldCorrection interface that are fixed in the latest version.

@effigies
Copy link
Member

effigies commented Feb 6, 2024

Tried this in #331. Getting CI to work is going to be a more serious maintenance task than it seemed worth putting in.

HippocampusGirl added a commit to HALFpipe/HALFpipe that referenced this pull request Feb 7, 2024
HippocampusGirl added a commit to HALFpipe/HALFpipe that referenced this pull request Feb 7, 2024
HippocampusGirl added a commit to HALFpipe/HALFpipe that referenced this pull request Feb 7, 2024
@lalalavi
Copy link
Contributor Author

lalalavi commented Feb 7, 2024

Hi @effigies I think we are going to try to correct the failing CI check. Should we continue on this current pull or reopen your old one? Also, I see that the maint/1.3.x where we are trying to merge already had build_docs failing. Is the build_n_pytest test fail the reason why you stopped trying to backport the fix?

@effigies
Copy link
Member

effigies commented Feb 7, 2024

Yes, let's not worry about docs. Just remove the stages from .circleci entirely at this point.

Need to figure out tests, though.

@lalalavi
Copy link
Contributor Author

lalalavi commented Feb 7, 2024

Hi @effigies, is it possible that the image for the last release 1.3.4 was uploaded in Docker Hub and then removed? Or maybe never uploaded? I see a tag in git, but there is no manifest in Dockerhub and I can also not pull it from the docker CLI, which is the point where build_n_pytest is starting to crash.

Also I think it would help me debug if i could see the report of circleci for the previous job ( https://app.circleci.com/pipelines/github/nipreps/sdcflows/jobs/5477 ) but it does't go further from loading. Is this due to authorization issues or does it happen to you too?

@effigies
Copy link
Member

effigies commented Feb 7, 2024

I think the 1.3.4 push to DockerHub failed. Old CircleCI runs expire, so we won't be able to see what happened that far back.

So we'll need to update the CircleCI config not to try gracefully fail pulls. You might look and see if we fixed this on master, but a straight copy from master to a legacy branch is rarely successful.

@lalalavi
Copy link
Contributor Author

lalalavi commented Feb 8, 2024

This commit fixes the build in build_n_pytest. It is not very pretty because it hardcodes the fallback option for this release, but if at some point the build of the 1.3.4 is pushed to dockerhub it will access that. It now seems to crash in one of the unit tests because it is missing the freesurfer license, which i also see is specified in the circle ci config. I will look into this a bit further today!

@HippocampusGirl
Copy link
Contributor

I think the build cache needs to be cleared, because the license will only be written out on install, not when restoring from cache

Found a cache from build 6344 at freesurfer-v1-
Size: 2.8 GiB
Cached paths:
  * /tmp/freesurfer

@effigies
Copy link
Member

effigies commented Feb 8, 2024

@lalalavi Pushed a quick commit, in case you didn't see.

@lalalavi
Copy link
Contributor Author

lalalavi commented Feb 9, 2024

That is awesome :) Now the test passes as well. Do you want to still remove the build_docs entirely or leave as is before merging?

HippocampusGirl added a commit to HALFpipe/HALFpipe that referenced this pull request Feb 14, 2024
@lalalavi
Copy link
Contributor Author

Hi Chris! I was wondering whether you still want me to remove the build_docs or whether you will merge it like it is now? I have not gone into it as I saw some other parts of the .yaml were depending on build_docs (the deploy_pypi) and thought it could generate more problems, but if you want it the job to disappear before merging let me know.

@effigies
Copy link
Member

Ah, I figured we would remove build_docs, but I can do that real quick.

@effigies effigies merged commit bc37d98 into nipreps:maint/1.3.x Feb 14, 2024
effigies added a commit that referenced this pull request Feb 14, 2024
1.3.5 (February 14, 2024)

Bug-fix release in 1.3.x series.

* FIX: Remove unused ANTs parameter that was removed in 2.4.1 (#431)
effigies added a commit to nipreps/fmriprep that referenced this pull request Jul 18, 2024
20.2.8 (July 18, 2024)

Bug-fix release in the 20.2.x LTS series.

We anticipate this being the final release in the 20.2.x LTS series.

* FIX: Select volumetric dseg.tsv from recent TemplateFlow releases (#3257)
* FIX: LTS package build (#3328)
* DOC: Read html_baseurl from RTD environment, if available (#3324)
* DOCKER: Pin conda environment more strictly (#2853)
* MNT: Require niworkflows ~1.3.6 (#2740)
* CI: Upgrade docker orb (#2865)

This release includes a number of fixes that have accumulated in niworkflows,
including the following fixes that affect fMRIPrep:

* FIX: Remove unused ANTs parameter that was removed in 2.4.1 (nipreps/sdcflows#431)
* FIX: Limit 3dQwarp to maximum 4 CPUs for stability reasons (nipreps/sdcflows#128)
* MAINT: Make call to scipy.stats.mode compatible with scipy 1.11.0 (nipreps/sdcflows#371)
* FIX: TSV2JSON should convert empty TSV files to empty JSON files (nipreps/niworkflows#747)
* FIX: Use copy function that does not preserve mtime when creating fsaverage
  directories (nipreps/niworkflows#703)
* FIX: Set pixdim[4] to match RepetitionTime (nipreps/niworkflows#679)
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.

3 participants