-
Notifications
You must be signed in to change notification settings - Fork 11
Add BCLConvert support #67
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 BCLConvert support #67
Conversation
matrulda
left a comment
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.
Great work! I left some comments for you!
Other general stuff:
- Could you adapt the test scripts for py 3.13 and make sure we use that version in GHA?
- We use Nextflow 24.10.4 on Miarka, can you update the tests to use that version?
- It would be nice to check if we can update some of the singularity containers used: https://github.com/Molmed/seqreports/blob/main/config/nextflow_config/singularity.config. But it might be out of scope for this PR. We could make a separate ticket about that. Let me know what you think!
8b93c07 to
6654bd6
Compare
|
I have now updated the code after fixing things from the comments in the scripts. |
f886e82 to
cb7cc63
Compare
a244e6a to
d54947b
Compare
|
I have been unable to update GHA python version to v3.13 due to this error ' It also seems that there are issues with lxml with python v3.13 but i also tried with python v.3.12 and it failed with the same error in this run I also could NOT update checkQC version in requirements.txt to match the latest checkQC singularity container available i.e v4.0.7 as i got this error ERROR: No matching distribution found for checkqc==4.0.7 |
Did you try to update the requirements when using py3.11? The example you sent was with 3.13. |
matrulda
left a comment
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.
Awesome! A few final comments.
I think the unit tests are currently running with python v3.11 successfully now. Or might i have missed anything from your question? |
cfd8863 to
1ea3cba
Compare
|
Thanks for the reviews. I have now pushed the updated code |
Ah sorry for being unclear! I meant, were you able to update the dependencies (e.g. CheckQC) when using py3.11? |
Oooh. Okay. I have just tried it and it failed with the same error [here](ERROR: No matching distribution found for checkqc==4.0.7) |
d3c10a0 to
314d899
Compare
|
I have now pushed the updated code changing outdir default back to 'Unaligned' in the Comment |
Ah I see now, 4.07 (and 4.1.0) that is on pypi has "Requires: Python <3.11, >3.10". I think this was due to the old interop version that we used. I think 3.13 is supported now? 🤔 We should investigate this. |
I see that checkQC uses interop v1.4.0 which supports python v3.8-3.13 as seen here. Does this mean we might need to build checkqc with the latest python version first to pypi before using it here? |
yeah, I think we need create a new checkqc release and put it on pypi. I guess we could make it 4.2.0-rc1 perhaps? Or is it perhaps only a patch update 🤔 |
I think it can be just a patch update because BCLConvert was introduced in 4.1.0 bt not used yet... so maybe 4.1.1-rc1? |
New release candidate out on pypi: https://pypi.org/project/checkQC/4.1.1rc1/ You can try if it works better! :) |
Thanks, i've now updated the checkQC version in the requirements file |
matrulda
left a comment
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.
Awesome, thank you!
Description:
Risk Analysis:
This potentially could break report generation and cause a stop in production, but validation in staging env should catch serious bugs like that.
Validation Procedure:
This version might not be tested separately/individually as just this single seqreports service but as a whole by triggering the arteria workflows and looking at the multiqc reports generated