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

DEV-2479: py38 upgrade #287

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from
Open

DEV-2479: py38 upgrade #287

wants to merge 21 commits into from

Conversation

jiakf
Copy link
Contributor

@jiakf jiakf commented Feb 7, 2024

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:

  • For macOS, py38 makes spawn the default method for multiprocessing as opposed to fork, 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.
  • Dropped some ubuntu/macOS versions from github actions as they are no longer supported.
  • macOS takes longer to start up a process now, and so a race condition was happening in the conftest file when running the Flask app in a separate process. Increasing the sleep time after starting the new process gave enough time for the app to come up before trying to make requests
  • the target function for Process also needs to be in the top-level module scope, so had to create a helper module function in conftest to start the Flask app.
  • Used the fixes in DEV 2469 fix github action #286 to fix the github actions workflows
  • FIxed github actions checkout step where setuptools_scm doesn't have the full git history to figure out the version

@jiakf jiakf changed the title DEV-2479: py38 upgrade POC DEV-2479: py38 upgrade Feb 16, 2024
@jiakf jiakf marked this pull request as ready for review February 19, 2024 16:04
Copy link
Member

@mpsolano mpsolano 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!

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
bin/package Outdated Show resolved Hide resolved
@jiakf jiakf force-pushed the feat/DEV-2479-upgrade-py38 branch 3 times, most recently from 7c67bac to 4a3ce19 Compare February 21, 2024 20:52
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.
@jiakf jiakf force-pushed the feat/DEV-2479-upgrade-py38 branch from 4a3ce19 to 43fc1e6 Compare March 26, 2024 16:58
@stilesj-uchicago stilesj-uchicago dismissed their stale review March 28, 2024 10:22

A lot has changed

@jiakf jiakf force-pushed the feat/DEV-2479-upgrade-py38 branch from 31a54b4 to 43fc1e6 Compare April 1, 2024 01:58
@@ -18,7 +18,7 @@
"pyOpenSSL~=18.0.0",
"PyYAML>=5.1",
"intervaltree~=3.0.2",
"importlib_metadata; python_version<'3.8'",
"importlib_metadata",

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

Copy link
Contributor Author

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

Copy link
Contributor Author

@jiakf jiakf Apr 2, 2024

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

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?

Copy link
Contributor Author

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.

@fgvieira
Copy link

fgvieira commented May 2, 2024

Any idea when this PR will be merged? Current bioconda recipe is broken and this PR might fix it:

[...]
  File "lib/python3.7/site-packages/python_utils/converters.py", line 89
    if match := regexp.search(input_):
              ^
SyntaxError: invalid syntax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants