search

nipreps / sdcflows

Susceptibility Distortion Correction (SDC) workflows for EPI MR schemes
https://www.nipreps.org/sdcflows
Apache License 2.0
32 stars 26 forks source link

FIX: Remove unused ANTs parameter that was removed in 2.4.1 #431

Closed lalalavi closed 9 months ago

lalalavi commented 9 months ago

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 commented 9 months ago

Tried this in https://github.com/nipreps/sdcflows/pull/331. Getting CI to work is going to be a more serious maintenance task than it seemed worth putting in.

lalalavi commented 9 months ago

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 commented 9 months ago

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

Need to figure out tests, though.

lalalavi commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

lalalavi commented 9 months ago

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?

lalalavi commented 9 months ago

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 commented 9 months ago

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