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

POC of new meta.yml structure and ontologies #5867

Closed
wants to merge 3 commits into from

Conversation

mirpedrol
Copy link
Member

@mirpedrol mirpedrol commented Jun 21, 2024

POC for nf-core/tools#3028 and nf-core/tools#3032

Run nf-core modules lint <module_name> --fix for modules:

  • fastp
  • samtools/sort
  • samtools/index
  • atlas/splitmerge
  • pear
  • multiqc
  • bwa/mem

Note that linting won't pass until #5837 is merged

ℹ️ The bio.tools ID was not automatically found for bwa.
ℹ️ The modules samtools/index, atlas/splitmerge and bwa/mem had wrong channels specified or missing inputs/outputs, thus the resulting modified meta.yml had to be fixed manually.

Ontology for input and output files were added for bwa/mem as an example.

homepage: "https://bitbucket.org/wegmannlab/atlas/wiki/Home"
documentation: "https://bitbucket.org/wegmannlab/atlas/wiki/Home"
tool_dev_url: "https://bitbucket.org/wegmannlab/atlas"
doi: "10.1101/105346"
licence: "['GPL v3']"
licence: ["GPL v3"]
identifier: biotools:atlas_db
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be atlas, not atlas_db in this case: https://bio.tools/atlas

Comment on lines 20 to 21
qualifier: val
type: map
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bentsherman - does this look about right to you, in terms of what we call things here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the use of adding the qualifier when you already have the type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The qualifier can be useful to know if two modules have the same inputs and outputs.
For example, we could have the case where two different outputs have type string, but the qualifier is val and env.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean to check if the modules can be chained? In that case it doesn't matter if it a val or env, as long as they are both strings

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am more thinking of a setting where modules can be interchangeable, for example for benchmarking of different tools. I would like to make sure that using aligner1 and using aligner2, both inputs and outputs will be exactly the same.
I know this can already be done with if/else and operating the channels if needed. But in the future, it would be cool if we can use this to automatically add any module which matches the requirements.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even there it doesn't matter if the string input is a val or env, as long as it is a string they are interchangeable.

The only difference is that val says "provide this value as a variable" whereas env says "provide this value as an environment variable", but those details are internal to the process

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see what you mean! Would it be best to remove the qualifier?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think so

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it!

@ewels
Copy link
Member

ewels commented Sep 17, 2024

Yeah, now you have merge conflicts 😓 Sorry @mirpedrol !

@mirpedrol
Copy link
Member Author

I have updated the branch and solved conflicts for all modules here #6015

@mirpedrol mirpedrol closed this Sep 18, 2024
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.

4 participants