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

chore: remove support for Python 3.7; prefer 3.8 #7329

Merged
merged 11 commits into from
Mar 4, 2024
Merged

Conversation

wes-turner
Copy link
Contributor

@wes-turner wes-turner commented Jul 6, 2023

Python 3.7 is EOL.

  • Some docs that referred to 3.7 now refer to 3.8
  • Training API "how to" doc no longer mentions any 3.7 docker images
  • CONTRIBUTING.md no longer says to upgrade pip when installing determined
  • CONTRIBUTING.md has revised text about Python requirements
  • mypy tests against 3.8.
  • Features new to 3.8 can be used without suppressing type checking or conditional imports. These changes have been made.
  • No longer run test-cli in CircleCI against Python 3.7.
  • There are no wheels for TF1 using Python 3.8. Removing 3.7 support means TF1 cannot be used. In previous commits, we removed support of TF1. We also clean up those references here. In particular:
    • e2e_tests/tests/command/test_run:test_k8_init_containers and e2e_tests/tests/command/test_run:test_k8_sidecars now run on TF2 images
    • Removed references to TF1 images in e2e_tests.
    • Removed references to TF1 Docker images in bumpenvs.yaml

Description

Test Plan

tests are green

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

@cla-bot cla-bot bot added the cla-signed label Jul 6, 2023
@netlify
Copy link

netlify bot commented Jul 6, 2023

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 214e138
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65e11de72b67650008d7c13f

@wes-turner wes-turner requested a review from a team as a code owner July 6, 2023 22:22
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Jul 6, 2023
@wes-turner wes-turner requested a review from a team as a code owner July 6, 2023 22:31
@wes-turner wes-turner changed the title test: mypy tests against python 3.8 chore: references to Python 3.7 now are for 3.8 Jul 6, 2023
Copy link
Contributor

@rb-determined-ai rb-determined-ai 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 to me!

There's still some 37 and 3.7 related things in the circleci config though.

@wes-turner wes-turner marked this pull request as draft July 6, 2023 22:58
@wes-turner
Copy link
Contributor Author

Converting to "draft" pending the removal of support for TF1 (and the TF1 environments that use Python 3.7). Wheels for TF are not available for Python 3.8 (https://pypi.org/project/tensorflow/1.15.5/)

@@ -35,7 +35,7 @@ and upgrade.
Installation
**************

The CLI is distributed as a Python wheel package and requires Python >= 3.7. We recommend setting up
The CLI is distributed as a Python wheel package and requires Python >= 3.8. We recommend setting up
a `virtualenv <https://virtualenv.pypa.io/en/latest/>`__ and using the ``pip`` utility to install
``determined`` into the environment:

Copy link
Contributor

@tara-det-ai tara-det-ai Jul 7, 2023

Choose a reason for hiding this comment

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

The command-line interface (CLI) is distributed in the form of a Python wheel package and requires Python version 3.8 or later.

(source: Microsoft Manual of Style)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

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

requested edits

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 47.19%. Comparing base (225dba3) to head (214e138).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7329   +/-   ##
=======================================
  Coverage   47.19%   47.19%           
=======================================
  Files        1155     1155           
  Lines      175116   175101   -15     
  Branches     2235     2237    +2     
=======================================
- Hits        82646    82639    -7     
+ Misses      92312    92304    -8     
  Partials      158      158           
Flag Coverage Δ
backend 42.36% <ø> (-0.01%) ⬇️
harness 63.71% <57.14%> (+<0.01%) ⬆️
web 42.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
harness/determined/cli/dev.py 50.27% <80.00%> (+0.54%) ⬆️
...ined/common/experimental/checkpoint/_checkpoint.py 69.58% <0.00%> (+0.74%) ⬆️

... and 2 files with indirect coverage changes

mypy used to test against python 3.7. 3.7 has passed EOL.
* unconditionally import functions added to typing in 3.8
* use copytree from shutils unconditionally (added in 3.8)
* remove reference to 3.7 environments image
* remove references to tf1 images from e2e_tests config
These Docker images are no longer called by reference in any current
test execution.
@wes-turner wes-turner marked this pull request as ready for review March 1, 2024 17:54
@wes-turner wes-turner requested review from a team as code owners March 1, 2024 17:54
@wes-turner wes-turner requested review from carolinaecalderon and caehd10 and removed request for caehd10 March 1, 2024 17:54
@wes-turner wes-turner changed the title chore: references to Python 3.7 now are for 3.8 chore: remove support for Python 3.7; prefer 3.8 Mar 1, 2024
Copy link
Contributor

@carolinaecalderon carolinaecalderon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -12,9 +12,6 @@ orbs:
win: circleci/[email protected]

executors:
python-37:
docker:
- image: python:3.7-slim-buster
python-38:
docker:
- image: python:3.8-slim-buster
Copy link
Contributor

Choose a reason for hiding this comment

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

We're still using the Buster images? :D

They've moved through Bullseye and are now on Bookworm. This is probably a separate PR, but those buster images haven't been updated for about a year. docker-library/python#822

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@dannysauer dannysauer left a comment

Choose a reason for hiding this comment

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

I didn't do a grep -R or anything, but this LGTM.

@wes-turner wes-turner merged commit fa856ab into main Mar 4, 2024
103 checks passed
@wes-turner wes-turner deleted the wes-bump-mypy-python branch March 4, 2024 17:23
@rb-determined-ai
Copy link
Contributor

yay, finally!

maxrussell pushed a commit that referenced this pull request Mar 21, 2024
Python 3.7 is EOL.

* Some docs that referred to 3.7 now refer to 3.8
* Training API "how to" doc no longer mentions any 3.7 docker images
* `CONTRIBUTING.md` no longer says to upgrade pip when installing determined
* `CONTRIBUTING.md` has revised text about Python requirements
* mypy tests against 3.8.
* Features new to 3.8 can be used without suppressing type checking or conditional imports. These changes have been made.
* No longer run `test-cli` in CircleCI against Python 3.7.
* There are no wheels for TF1 using Python 3.8. Removing 3.7 support means TF1 cannot be used. In previous commits, we removed support of TF1. We also clean up those references here. In particular:
  - `e2e_tests/tests/command/test_run:test_k8_init_containers` and  `e2e_tests/tests/command/test_run:test_k8_sidecars` now run on TF2 images
  - Removed references to TF1 images in `e2e_tests`.
  - Removed references to TF1 Docker images in `bumpenvs.yaml`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants