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

Use conda integrated package management feature within snakemake #351

Merged
merged 19 commits into from
Feb 5, 2025

Conversation

Dhananjhay
Copy link
Collaborator

@Dhananjhay Dhananjhay commented Jan 20, 2025

Excited to report that I successfully ran an entire workflow of hippunfold (for modality T1w) on a test dataset (lowresMRI) using the --use-conda flag and without --use-singularity. I ran it once on the CBS server and another time on a vanilla Ubuntu 24.04.1 LTS Docker container.

To run the full end-to-end pipeline, I had to make some fundamental changes, some of which have their own dedicated PRs, and others need immediate review here:

  • Since the Equivolume optoin uses equivolumetric layering method from the nighres package, which has proven difficult to integrate into the Conda ecosystem, I temporarily switched the default option of --laminar_coords_method in snakebids.yaml from equivolume to laminar. This change is still a work in progress (WIP).
  • Laynii native WIP - Laynii native #349 — We expect a robust solution soon!

Due to conflicting versions of libitk required by ants v2.5.1 and c3d v1.4.2, I had to replace antsApplyTransforms with greedy in the one instance where they were used together. The newest version of ants available on conda-forge is v2.5.4, which has a compatible libitk version with c3d, but it currently segfaults during runtime. This issue is being actively investigated:

The issue can be reproduced at subfields.smk#L47.

Update

  • Also worked for T2w modality (used thickSlice dataset)

@Dhananjhay Dhananjhay added enhancement New feature or request v2.0 labels Jan 20, 2025
@Dhananjhay Dhananjhay self-assigned this Jan 20, 2025
@Dhananjhay Dhananjhay requested review from akhanf and jordandekraker and removed request for akhanf January 20, 2025 16:10
@akhanf
Copy link
Member

akhanf commented Jan 20, 2025

This is amazing! Thanks for pushing this through..
I've made some comments on suggestions on what to name the various numbered envs for easier maintainance/development in the future, and where things could be merged/dropped too..

@akhanf
Copy link
Member

akhanf commented Jan 21, 2025

I've merged in #345 now, which means this will have some conflicts to resolve now.. I'll try to do that now (should be pretty straightforward, just alot of things moving around or being removed in that PR)..

@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

Got a chance to try this out with conda!

I installed miniconda, then installed snakemake (instructions from snakemake docs), and in that conda env then did conda install snakebids and then conda install ruamel.yaml (was missing from snakebids, I recall you have an open snakebids issue on something related?)

Then I just ran hippunfold with ./hippunfold/run.py directly (rather than pip installing as I didn't want to install any more dependencies, but I am guessing we can also make a hippunfold conda package (that depends on snakebids, snakemake).

Was on my way, but then I realized that dev-v2.0.0 now depends on a new dev version of workbench (for the new hippocampus cifti integration), so for now we may just have to use the container for those rules (otherwise they will fail).. unless we want to make a temporary workbench conda package for the dev build?

@Dhananjhay
Copy link
Collaborator Author

I installed miniconda, then installed snakemake (instructions from snakemake docs), and in that conda env then did conda install snakebids and then conda install ruamel.yaml (was missing from snakebids, I recall you have an open snakebids issue on something related?)

Right, there's an open PR that resolves the issue with ruamel.yaml. It's ready to be merged—just needs your approval!

Then I just ran hippunfold with ./hippunfold/run.py directly (rather than pip installing as I didn't want to install any more dependencies, but I am guessing we can also make a hippunfold conda package (that depends on snakebids, snakemake).

I just uploaded snakebids v0.13.1 to Bioconda, and the required snakemake versions are already available. So, we're in good shape to publish hippunfold v2.0.0 on Conda!

Was on my way, but then I realized that dev-v2.0.0 now depends on a new dev version of workbench (for the new hippocampus cifti integration), so for now we may just have to use the container for those rules (otherwise they will fail).. unless we want to make a temporary workbench conda package for the dev build?

The --use-singularity flag takes precedence over the --use-conda flag, so any rules with the container directive will default to using Singularity containers instead of Conda environments. I agree—we could temporarily host the new dev version of Workbench on our custom khanlab Conda repository until the package authors release it on Conda-Forge. Is the latest dev version available on Workbench's official GitHub repository? The only copy I could find seems to be hosted on Dropbox

@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

I installed miniconda, then installed snakemake (instructions from snakemake docs), and in that conda env then did conda install snakebids and then conda install ruamel.yaml (was missing from snakebids, I recall you have an open snakebids issue on something related?)

Right, there's an open PR that resolves the issue with ruamel.yaml. It's ready to be merged—just needs your approval!

I'll have a look at that now - if we can get the tests passing then can merge it in

Then I just ran hippunfold with ./hippunfold/run.py directly (rather than pip installing as I didn't want to install any more dependencies, but I am guessing we can also make a hippunfold conda package (that depends on snakebids, snakemake).

I just uploaded snakebids v0.13.1 to Bioconda, and the required snakemake versions are already available. So, we're in good shape to publish hippunfold v2.0.0 on Conda!

Great - but no need to publish a release yet, unless you want to publish some test releases to iron out the process..

Was on my way, but then I realized that dev-v2.0.0 now depends on a new dev version of workbench (for the new hippocampus cifti integration), so for now we may just have to use the container for those rules (otherwise they will fail).. unless we want to make a temporary workbench conda package for the dev build?

The --use-singularity flag takes precedence over the --use-conda flag, so any rules with the container directive will default to using Singularity containers instead of Conda environments. I agree—we could temporarily host the new dev version of Workbench on our custom khanlab Conda repository until the package authors release it on Conda-Forge. Is the latest dev version available on Workbench's official GitHub repository? The only copy I could find seems to be hosted on Dropbox

Sure we could post the temporary one in our khanlab channel - it's not on their github as its not a release yet, it was passed along to us here https://balsa.wustl.edu/myelin and I put it on my dropbox for ease of download url.

@Dhananjhay Dhananjhay mentioned this pull request Jan 30, 2025
@Dhananjhay
Copy link
Collaborator Author

Great - but no need to publish a release yet, unless you want to publish some test releases to iron out the process..

I've published a release based on integrate-conda branch on the custom khanlab for the moment; don't forget to add channels bioconda and conda-forge in your local conda configuration before installing it. I was also able to run an entire workflow for modality T1w using this package 🥳 Once we release v2.0.0 it'd take no time to publish an official release to the bioconda repo.

Sure we could post the temporary one in our khanlab channel - it's not on their github as its not a release yet, it was passed along to us here https://balsa.wustl.edu/myelin and I put it on my dropbox for ease of download url.

In the latest commit, I've reverted back to the latest workbench labels and modified workbench.yaml to use dev-connectome-workbench package hosted on our khanlab conda repo. I tested these changes by running the following command:

hippunfold lowresMRI/ test-lowresMRI participant --participant-label 01 --modality T1w --use-conda --cores all --output_density 0p5mm 2mm unfoldiso --autotop-labels hipp

I'm planning on following up this with more extensive testing, more specifically by running the wet-test run script written by Jordan.

@akhanf
Copy link
Member

akhanf commented Feb 3, 2025

OK great - I'll merge upstream changes in and resolve some conflicts, but then I think we could merge this PR in?

@Dhananjhay
Copy link
Collaborator Author

Yes, I think we are ready to merge!

@akhanf
Copy link
Member

akhanf commented Feb 3, 2025

actually, I'm going to merge in #359 then #349 first, so workflow doesn't rely on nighres..

NOTE: laynii environment needed, and pyvista (with new pygeodesic) not
actually tested yet..
@akhanf
Copy link
Member

akhanf commented Feb 4, 2025

ok got things up to date with upstream -- just need to add in a new laynii environment (I already refer to it as laynii.yaml), and re-run a wet test to make sure nothing is broken..

@Dhananjhay
Copy link
Collaborator Author

I already had put laynii on our custom khanlab repo weeks ago so we don't have to wait to start testing!

@akhanf
Copy link
Member

akhanf commented Feb 5, 2025

everything here looks good so merging it in!

@akhanf akhanf merged commit af93bf5 into dev-v2.0.0 Feb 5, 2025
6 checks passed
@akhanf akhanf deleted the integrate-conda branch February 5, 2025 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants