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

Add janis converter #73

Merged
merged 13 commits into from
Jan 21, 2021
Merged

Conversation

illusional
Copy link
Contributor

@illusional illusional commented Jan 17, 2021

A few questions still to answer:

  • Optional nested types, eg: String?[]
  • Binding the prefix to each element of an array?
  • Tool arguments (maybe this isn't applicable)

@multimeric
Copy link
Collaborator

multimeric commented Jan 18, 2021

Optional nested types, eg: String?[]

If this means "a list of optional strings", then no, aCLImatise doesn't support that. Can you think of an example CLI tool that has such a type? It might be a future direction to move the required/optional into the type object, since it's currently in the Flag/Positional

Binding the prefix to each element of an array?

As above. Could be supported but would need an example

Tool arguments (maybe this isn't applicable)

What do you mean by this?

@illusional
Copy link
Contributor Author

Optional nested types, eg: String?[]

I looked through our tool definitions, I don't have an example for this.

Binding the prefix to each element of an array?

Yep, Gatk4ApplyBQSR:

gatk4 ApplyBqsr -I bam -L interval1.bed -L interval2.bed

Tool arguments

In Janis, these are:

  1. "Unchangeable parameters" (eg: --runMode alignReads) for our StarAlignReads tool, or
  2. "where you should perform some expression" (similar to CWL's valueFrom, we insist on doing this operation in an argument. But don't need these either.

@multimeric
Copy link
Collaborator

Okay, both of those remaining points could conceivably be parsed from the help text, so I might eventually get around to them. But since I don't support that yet you don't have to worry about converting them for now.

@illusional
Copy link
Contributor Author

Hi @TMiguelT, it looks like the CI is failing with an isort error. It's trying to move the deprecated import, though when I ran isort locally this is where it placed it -> I'm running VERSION 5.7.0 of isort if that helps. Not sure what to do because moving the import manually will cause my precommit hook to fail.

@multimeric
Copy link
Collaborator

Oh yes this is a strange and annoying error. I've been chucking the third party libs in the .isort.cfg which seems to sort it.

@illusional
Copy link
Contributor Author

Hey @TMiguelT, tests are passing now. Are there any other steps to follow?

@multimeric
Copy link
Collaborator

Nope, just my (upcoming) code review, and it might be worth eyeballing the outputs to see if they make sense (and incorporating anything you note into the validation function)

Copy link
Collaborator

@multimeric multimeric left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few comments. Incidentally I would appreciate type annotations wherever possible in the JanisGenerator class, cheers.

aclimatise/converter/janis.py Outdated Show resolved Hide resolved
aclimatise/converter/janis.py Outdated Show resolved Hide resolved
aclimatise/converter/janis.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
test/util.py Show resolved Hide resolved
I was hoping to avoid importing janis_core at the start to avoid any overhead. But less avoidable for type annotations.
@illusional illusional marked this pull request as ready for review January 21, 2021 06:58
@illusional
Copy link
Contributor Author

Thanks for the feedback @TMiguelT, I've addressed your feedback by importing janis_core at the top (rather than only when initialised), it provides the benefit of type annotations at the expense of some extra loading time of janis_core (but that shouldn't impact this).

@multimeric
Copy link
Collaborator

At some point I'll work out a system that only conditionally imports the generator modules, meaning that users who don't want Janis won't have to install or load the Janis module. But you shouldn't have to worry about that. Also fwiw I believe you could have done the type annotations using strings to prevent the need for a global import.

Anyway, it's fine as it is.

@illusional
Copy link
Contributor Author

Tests all pass :)

@multimeric
Copy link
Collaborator

Great!

@multimeric multimeric merged commit 197b0bd into aCLImatise:master Jan 21, 2021
@multimeric
Copy link
Collaborator

Hey @illusional, I've had this merged into the Base Camp for a while, but I note that it's generating the Python class all on one line, e.g. https://aclimatise.github.io/BaseCamp//packages/samtools/1.11/samtools_dict/samtools_dict.py. Is there a simple fix for this?

@illusional
Copy link
Contributor Author

Thanks for question @TMiguelT! If the black formatted is installed, it will automatically format :)

@multimeric
Copy link
Collaborator

As in I just have to passively have it installed when the Janis convert is run?

@illusional
Copy link
Contributor Author

Yep!

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.

None yet

2 participants