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

Drop support for Python 3.8 #2500

Merged
merged 7 commits into from
Nov 15, 2024
Merged

Drop support for Python 3.8 #2500

merged 7 commits into from
Nov 15, 2024

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented Nov 13, 2024

Closes CLI-10

Changelog

  • Modal no longer supports Python 3.8, which has reached its official EoL.

@@ -1063,7 +1063,7 @@ def get_hash(img: Image) -> str:
spec_file=test_dir / "supports" / "test-conda-environment.yml",
channels=["conda-forge", "my-channel"],
)
assert get_hash(img) == "d9d4c9fe24769ce587877b9752a64486e8f7d8520731110bd2fa666de82f43fd"
assert get_hash(img) == "f8701ce500d6aa1fecefd9c2869aef4a13c77ab03925333c011d7eca60bbf08a"
Copy link
Contributor Author

@mwaskom mwaskom Nov 13, 2024

Choose a reason for hiding this comment

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

This isn't a real change to the Image recipe, but the test-conda-environment.yml support file we use had python_version: 3.8, which I've updated to avoid other issues.

Comment on lines +85 to +95
- uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, we were always running this check on 3.8 anyway...

@mwaskom mwaskom force-pushed the michael/drop-py38 branch 5 times, most recently from cae63bb to 9a8ffe5 Compare November 13, 2024 22:11
@mwaskom mwaskom requested a review from freider November 14, 2024 15:38
@erikbern
Copy link
Contributor

looks good! Looking forward to removing the from typing import shortly

Copy link
Member

@ekzhang ekzhang left a comment

Choose a reason for hiding this comment

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

I know you asked people to ✔️ but I'll let @freider 😅

@freider
Copy link
Contributor

freider commented Nov 15, 2024

🪓

@@ -91,7 +103,13 @@ jobs:
run: |
python -m venv venv
source venv/bin/activate
pip install grpcio-tools==1.59.2 grpclib==0.4.7
if [ "${{ matrix.python-version }}" == "3.9" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Our published package has the same generated protos regardless of the python version of the client that ends up using it right? So maybe this should use the same python and versions as we do when publishing? (a bit messy to switch versions etc, but relatively easy with uv as a python manager)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is just meant to assert that we haven't added any third-party dependencies to python -m modal._container_entrypoint that aren't in the expected container requirements, so the specific way that we compile the protos isn't super important because whatever choice we make wouldn't introduce transitive dependencies. But (as we're discussing on Slack) there definitely are larger and trickier questions about our proto packaging!

Copy link
Contributor

@freider freider left a comment

Choose a reason for hiding this comment

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

lgtm! But I think you need to bump the minor version again (?)

@mwaskom mwaskom merged commit 16b7f7f into main Nov 15, 2024
21 checks passed
@mwaskom mwaskom deleted the michael/drop-py38 branch November 15, 2024 15:32
@mwaskom
Copy link
Contributor Author

mwaskom commented Nov 15, 2024

But I think you need to bump the minor version again (?)

Yeah the workflow where subsequent commits introduce a merge conflict with the "reset the micro version" diff is annoying. I've been meaning to automate that (i.e. just detect a minor version bump and handle the micro version accordingly) in the CD workflow but it doesn't happen quite often enough to make that feel worthwhile...

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.

4 participants