-
Notifications
You must be signed in to change notification settings - Fork 1
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
[DEV] Adding a gatk_jar downloader #58
Conversation
Thanks for the initiative Davi! 💚 👍 I'm also working on addressing the installation issue via conda and perhaps we can merge this PR as soon as the problem with |
😃 sure! this problem is so odd, I'm interested on how they'll solve it. |
c837e9b
to
b3463a9
Compare
582f620
to
0da326b
Compare
Update: I added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Davi thanks for this PR and the initiative on fixing these usability issues.
I have been testing the mtbseq-1.0.4
release via (i) conda 🔴 (ii) docker 🔴 and basically if gatk/gatk3
is not the issue then perl Basic::Statistics
module is throwing dependency issues!
Even after using conda!!!
Therefore I'd like to be extra sure andd confirm whether you've tested this on a baseline linux machine via conda
as well?
conf/docker.config
Outdated
withName: | ||
'DOWNLOAD_GATK_JAR' { | ||
container = 'alpine:3.14' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mxrcon , perhaps we might not need to use an extra container?
In these cases we can reuse (maybe?) the mtbseq
container since its derived from debian
and would be downloaded (and available) on the underlying machine anyhow.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, even alpine being a minimal container, we can use the mtbseq for it.
gatk38_jar = "${projectDir}/resources/GenomeAnalysisTK-3.8-0-ge9d806836/GenomeAnalysisTK.jar" | ||
|
||
gatk_jar_link = "https://storage.googleapis.com/gatk-software/package-archive/gatk/GenomeAnalysisTK-3.8-0-ge9d806836.tar.bz2" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some testing, preferably we can remove the necessity of gatk38_jar
completely 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, but we can keep it until we properly test it
Davi, let's put this PR on hold till the point our analysis for generated reports is completed and then we can merge and test this PR and make the |
@abhi18av Sure! I completely agreed. |
@abhi18av
|
Okay thanks for the confirmation Davi, is it working fine with |
@abhi18av, yes! using the version 1.0.3 it works with no problems when using |
Okay, then please elaborate step-by-step how you tested with I just want to confirm whether the default conda setup for that version works with Happy to connect tonight if this is not clear. |
@abhi18av, Here's how i setup the testings, You can change the version 1.0.3:
Output:
version 1.0.4:
output:
Very odd, both of them return errors 😭 version 1.0.3 + gatk:
output:
LATEST WORKING SOLUTION: version 1.0.3 + gatk:
output:
|
This sounds good Davi, it seems that we've both been able to test the conda based setup independently for Adding here the final working solution ✅
|
This PR will be addressed after the QC workflow has been confirmed to be working. |
On my last review around our overall progress on this task, I'd say that this PR is very close to #74, and the overall design of the custom docker container and the conda_env will take care of this problem. Before I start to tweak those changes, I'd like to hear your thoughts @abhi18av : What do you think about this:
This way, well have both On both changes, we'll need to update the installation instructions so the user will need to pick one of the following:
Perhaps, we should publish the resulting image so we can use |
Closing this PR due to the decision to improve the GATK jar using #74 |
Hey 👋, May I help on this issue? #57 I'm adding a utility module able to download the Gatk_jar using the
wget
command and them usetar
to uncompress it. The output is caught by a glob pattern, Not sure about this addition, maybe we can discuss another way of parsing it!I've also added the alpine container usage on the docker profile, it has tar and wget. (i've double-checked on alpine package list.
This only add the module as utility, we can discuss if the module should be added on our workflows.
This Pull Request closes #57
Hope this small PR helps!
Feel free to add your thoughts, I'm available to help with this task.
Kindly, Davi