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

FIX: enable multi-processing on Linux #33

Merged
merged 4 commits into from
Jan 4, 2023

Conversation

misialq
Copy link
Contributor

@misialq misialq commented Dec 22, 2022

Closes #32.

Unfortunately, multiprocessing seems not to be working on macOS, hence the platform check. See ablab/quast#175.

@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #33 (b7c6b87) into main (a562844) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #33   +/-   ##
=======================================
  Coverage   98.66%   98.66%           
=======================================
  Files          25       25           
  Lines        1269     1275    +6     
=======================================
+ Hits         1252     1258    +6     
  Misses         17       17           
Impacted Files Coverage Δ
q2_assembly/__init__.py 100.00% <ø> (ø)
q2_assembly/_action_params.py 100.00% <ø> (ø)
q2_assembly/_utils.py 100.00% <ø> (ø)
q2_assembly/bowtie2/__init__.py 100.00% <ø> (ø)
q2_assembly/bowtie2/indexing.py 100.00% <ø> (ø)
q2_assembly/bowtie2/mapping.py 100.00% <ø> (ø)
q2_assembly/bowtie2/tests/test_indexing.py 97.61% <ø> (ø)
q2_assembly/bowtie2/tests/test_mapping.py 99.21% <ø> (ø)
q2_assembly/bowtie2/tests/test_utils.py 98.92% <ø> (ø)
q2_assembly/bowtie2/utils.py 97.95% <ø> (ø)
... and 14 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@misialq
Copy link
Contributor Author

misialq commented Dec 22, 2022

Hey @gregcaporaso, what do you think about that? It would at least make it use multiprocessing on Linux for now...

Copy link
Contributor

@gregcaporaso gregcaporaso left a comment

Choose a reason for hiding this comment

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

@misialq, this looks good to me. Just a couple of suggestions on the code.

Too bad it won't work on macOS right now! I would put a link to that issue (ablab/quast#175) in your code as a reminder of where to look to see if it's been resolved in the future - for example:

# This is a limitation in quast that is documented in this issue:
# https://github.com/ablab/quast/issues/175

q2_assembly/quast/quast.py Outdated Show resolved Hide resolved
q2_assembly/quast/quast.py Outdated Show resolved Hide resolved
q2_assembly/quast/tests/test_quast.py Show resolved Hide resolved
@misialq
Copy link
Contributor Author

misialq commented Jan 4, 2023

Thanks for review @gregcaporaso! All updated.

@misialq misialq requested a review from gregcaporaso January 4, 2023 10:15
Copy link
Contributor

@gregcaporaso gregcaporaso left a comment

Choose a reason for hiding this comment

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

This looks good to me @misialq! Do you want me to merge this, or will you take care of it on that end? (I haven't merged anything in this repo yet, so just want to check on what would be most helpful.)

@misialq
Copy link
Contributor Author

misialq commented Jan 4, 2023

I can take it from here, thanks! 🙏

@misialq misialq merged commit 87b5e02 into bokulich-lab:main Jan 4, 2023
@misialq misialq deleted the issue-32-quast-multithreading branch January 4, 2023 15:05
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.

Enable multiprocessing in evaluate-contigs
2 participants