-
Notifications
You must be signed in to change notification settings - Fork 41
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-2479: py38 upgrade #287
base: develop
Are you sure you want to change the base?
Conversation
cb52cb3
to
01f1957
Compare
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.
Looks good!
7c67bac
to
4a3ce19
Compare
This updates DTT to run with py38 and drops support for earlier python versions. This change also includes running pyupgrade on the repository and updating the github actions to run on supported operating systems.
4a3ce19
to
43fc1e6
Compare
31a54b4
to
43fc1e6
Compare
@@ -18,7 +18,7 @@ | |||
"pyOpenSSL~=18.0.0", | |||
"PyYAML>=5.1", | |||
"intervaltree~=3.0.2", | |||
"importlib_metadata; python_version<'3.8'", | |||
"importlib_metadata", |
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.
Is this right?
isn't importlib_metadata
already in py3.8? (so this should not be needed?)
https://docs.python.org/3/library/importlib.metadata.html#module-importlib.metadata
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.
i see that now, let me try building without importlib_metadata
included in setup.py
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.
So the importlib library was made part of python3.8, but the syntax will have to change where it is used in this repo. Additionally, the features of importlib_metadata that were included in python3.8 are equivalent to importlib_metadata version 1, whereas this was already tested with importlib_metadata version 7.0.1. I'd recommend keeping the backport library in here as a result.
@@ -1,5 +1,6 @@ | |||
-c requirements.txt | |||
|
|||
click>= 8 |
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.
You sure you want to support any greater than 8? Like version 9 when it comes out? Or do you want ~=8.0
?
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.
i think in the future if a newer version breaks functionality we can pin it then.
Any idea when this PR will be merged? Current bioconda recipe is broken and this PR might fix it:
|
This PR also includes running pyupgrade for py38 on the repository, so most of the file changes are from that. The main fixes in this PR:
spawn
the default method for multiprocessing as opposed tofork
, so the code as written no longer worked for macOS. For now, we are switching DTT to use threads on macOS as opposed to processes, so the same as what is done on Windows. Some performance testing was done to compare the threads vs process versions of macOS DTT, and the performance was comparable.