-
Notifications
You must be signed in to change notification settings - Fork 189
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
installing missing modules from other repos #2497
Comments
So I think that the actual errors in the second part are unrelated to the warnings from the first. One thing I am intrigued by though is Is there any chance you can share the subworkflow main.nf file? |
Are you able to supply your |
The module isn't actually nested more than once, nf-core/tools is just parsing it wrong when it comes to spark tools for some reason? Here's my imports in the main.nf file: It also didn't allow me to write gatk4/markduplicatesspark in the meta.yml, it gave a linting error unless i added the extra slash. you think that is the issue? i thought it was separate but i can look at that further. |
Sure!
After this step i get:
|
I have a feeling that |
Back to the actual issue at hand. The So I tried adding it and
Would you be able to copy a module used in the subworkflow into the |
i've opened a different branch called tools/nf_core/modules/modules_json.py Line 1169 in 9ab896c
and here's the updated modules.json the underlying issue here is that there's no way to tell nf-core where a given module ACTUALLY came from -- in fact nf-core/tools ignores org_path (msk vs nf-core) information in the subworkflow definition. the modules.json is actually highly specific but grabbing the dependencies of the subworkflow is not. if we had more specific dependency definition i think that might be more robust way to handle them. |
after adding the new subworkflow my modules.json file looks like this:
and trying to install anything else indeed gives me: |
Ok yes. You have nailed the issue here. That I would fully support a better way of defining dependent modules, but it would require buy-in from both tools devs and the wider nf-core community, because it would affect how In theory we already track this information in the components:
- gatk4/mutect2
- gatk4/genomicsdbimport
- gatk4/createsomaticpanelofnormals I would be in favour of leveraging this to determine the dependencies, but also to allow us to provide remotes. The major issue within |
I've thought about what it would be like to use the meta.yml as well, something like this:
where git-remote and org_path are optional and assumed to be "self" if not explicitly defined. commit sha and branch can be optionally defined as well, but otherwise can have default behavior. it would be the burden of whoever is adding or updating the subworkflow to add this information if they care about it. i think this approach may actually be achievable. but even if it does not have buy-in from the tools devs and the wider community, i still think it would be beneficial to everyone to handle missing dependencies more gracefully. currently the error is somewhat unhelpful and doesn't even state the subworkflow that i have to remove if i want to fix the problem. agree that what i came up with for testing is not ideal and quite hacky, but by allowing users to start defining dependencies, i think that's when other testing solutions might start to follow. if no one can use hybrid subworkflows then no one will tackle the question of testing it either. |
i have outlined some steps that i think are achievable:
As you pointed out earlier, testing would not be fully accounted for here, but until a solution is created, nf-core/modules can be configured to not accept hybrid subworkflows, while separate institutions can have the flexibility to create these subworkflows if they desire. Please let me know your thoughts! |
So I agree mostly with the solution here, but I do see some issues.
I don't think One possible route to making this work more easily would be for saubworkflows to package their modules with them (something I think @ewels supports in theory) but that comes with it's own headaches for tooling and (I literally just realised) would create a nightmare hellscape if we intend to support nested subworkflows still (which we currently do but I'm not sure is really wise). |
What's wrong with module file inclusion with nested subworkflows? It's just |
It's more just the arbitrary depth nesting you could potentially end up with. So
And then you have to have code that can efficiently traverse this tree and keep track of versions. Maybe it's not as bad as I'm imagining but that deeply nested structure just isn't that nice imo |
i'm not sure i fully understand how the nested subworkflows are supposed to work, probably because i haven't been present for previous discussions. maybe this is has already been discussed and decided against, but what about testing components using a fresh repo? meaning, instead of testing the modules repo code base directly: create a new pipeline project using whether or not hybrid workflows become a reality or not, should we attempt to fix the more specific issue that we've identified in this thread? i wanted to come back to what i wrote before:
|
Description of feature
i am attempting to install a subworkflow from a custom repository, but i am having issues installing the missing modules from the nf-core/modules repo:
the subworkflow installs but I get a few errors telling me i should install certain modules. however when i attempt to the install them from nf-core i get weird and uninformative errors:
i may be wrong, but i think this is happening due to the recreate_dependencies code:
tools/nf_core/modules/modules_json.py
Lines 1158 to 1181 in 9ab896c
I know that nf-core is currently not supporting hybrid-repo functionality. still, i was wondering if it might make sense to skip or throw a warning, rather than make the tool quit altogether. OR filter out dependencies that require modules from other repos and exclude them from any checking? not sure how this will play out in the broader sense.
The text was updated successfully, but these errors were encountered: