Skip to content

DEV-2479: py38 upgrade - #287

Merged
jiakf merged 1 commit into
developfrom
feat/DEV-2479-upgrade-py38
Jun 26, 2024
Merged

DEV-2479: py38 upgrade #287
jiakf merged 1 commit into
developfrom
feat/DEV-2479-upgrade-py38

Conversation

@jiakf

@jiakf jiakf commented Feb 7, 2024

Copy link
Copy Markdown
Contributor

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 force-pushed the feat/DEV-2479-upgrade-py38 branch from cb52cb3 to 01f1957 Compare February 16, 2024 16:14
@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

@mpsolano mpsolano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

Comment thread .github/workflows/ci.yaml Outdated
Comment thread .github/workflows/ci.yaml Outdated
Comment thread bin/package Outdated
@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
@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
Comment thread setup.py
Comment thread dev-requirements.in
@fgvieira

fgvieira commented May 2, 2024

Copy link
Copy Markdown

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

@qqiao2024 qqiao2024 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested download on my mac (os 14) with the artifact for macos 12 and amazon linux with the artifact for ubuntu 20.

Both are working.

The logging timestamp is not working, we need to test again after that change is merged.

@jiakf
jiakf force-pushed the feat/DEV-2479-upgrade-py38 branch 2 times, most recently from 5748956 to d0c7f80 Compare June 25, 2024 19: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 d0c7f80 to a3380f7 Compare June 26, 2024 16:22
@jiakf
jiakf merged commit a32dfd1 into develop Jun 26, 2024
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.

7 participants