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: enforce supported Python versions on pip installation #19740

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

rexledesma
Copy link
Contributor

@rexledesma rexledesma commented Feb 12, 2024

Summary & Motivation

Resolves #17933.

How I Tested These Changes

  • Create a virtualenv of Python 3.12, see installation fail.
  • Create a virtualenv of Python 3.7, see installation fail.
  • Create a virtualenv of Python 3.8, see installation succeed.
  • See Buildkite still succeed.

Copy link
Contributor Author

rexledesma commented Feb 12, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@rexledesma rexledesma requested a review from gibsondan February 12, 2024 17:12
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

I think an upstream package change to unversal_pathlib is about to make dagster Just Work on python 3.12

@rexledesma
Copy link
Contributor Author

I think an upstream package change to unversal_pathlib is about to make dagster Just Work on python 3.12

I'll wait until #17350 is closed and I'll bump up the pin to 3.12. Then we can merge this!

@gibsondan
Copy link
Member

Upstream release has landed, adding buildkite support in #19588 (need to rebuild some images which take a while, so might not land today)

Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

back to your queue

@rexledesma
Copy link
Contributor Author

@gibsondan I just added the same constraint on all packages, since this will be easier to maintain than manually auditing each package against its Buildkite test suite. Feel free to push back on that though.

The main issue that we want to defend here is folks breaking themselves if they try to use a breaking edge Python version that we're not even testing the main dagster against (e.g. Python 3.13/3.14).

@alangenfeld
Copy link
Member

Python 3.7, see installation fail

do we know things fail if a user happens to try to use 3.7 without this? Just curious if this specific change will be what stops some random 3.7 user who happens to be fine otherwise.

@rexledesma
Copy link
Contributor Author

Python 3.7, see installation fail

do we know things fail if a user happens to try to use 3.7 without this? Just curious if this specific change will be what stops some random 3.7 user who happens to be fine otherwise.

Unknown, since we don't test against that suite anymore. FWIW, 3.7 has been EOL for 8 months now.

@alangenfeld
Copy link
Member

did a quick test in a clean 3.7 env for 1.6.5 and we blow up immediately so think were good on that front

(1.6.5-3.7) ~/dagster:master$ dagster dev
Traceback (most recent call last):
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/bin/dagster", line 5, in <module>
    from dagster.cli import main
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/__init__.py", line 98, in <module>
    from dagster._config.pythonic_config import (
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_config/pythonic_config/__init__.py", line 1, in <module>
    from .config import (
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_config/pythonic_config/config.py", line 27, in <module>
    from dagster._core.definitions.definition_config_schema import (
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/definitions/__init__.py", line 88, in <module>
    from .repository_definition import (
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/definitions/repository_definition/__init__.py", line 1, in <module>
    from .repository_definition import (
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/definitions/repository_definition/repository_definition.py", line 19, in <module>
    from dagster._core.definitions.asset_graph import AssetGraph, InternalAssetGraph
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/definitions/asset_graph.py", line 25, in <module>
    from dagster._core.definitions.asset_subset import ValidAssetSubset
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/definitions/asset_subset.py", line 15, in <module>
    from dagster._core.definitions.partition import (
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/definitions/partition.py", line 31, in <module>
    from dagster._core.definitions.run_request import (
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/definitions/run_request.py", line 22, in <module>
    from dagster._core.instance import DynamicPartitionsStore
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/instance/__init__.py", line 51, in <module>
    from dagster._core.log_manager import DagsterLogRecord
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/log_manager.py", line 9, in <module>
    from dagster._core.utils import coerce_valid_log_level, make_new_run_id
  File "/Users/alangenfeld/venvs/repros/1.6.5-3.7/lib/python3.7/site-packages/dagster/_core/utils.py", line 10, in <module>
    from typing import (
ImportError: cannot import name 'TypedDict' from 'typing' (/Users/alangenfeld/.pyenv/versions/3.7.8/lib/python3.7/typing.py)

just added the same constraint on all packages

torn on this - agree its easy to miss keeping it up to date but would be nice to be accurate

Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

so the tradeoff here, much like #11890 - is that there is a hypothetical future where dagster is compatible with 3.13 out of the box (maybe once some upstream dependencies upgrade) but this prevents you from installing it. Maybe that's unlikely enough (and python version upgrades are rare enough) that we don't care?

@alangenfeld
Copy link
Member

  • @smackesey for another opinion to hopefully unstick indecision

based on the past several new pythons, the probability of old versions working on 3.13 unmodified quite low

@smackesey
Copy link
Collaborator

I think this is a good idea. Though I argued heavily against pre-emptive capping of dependencies in the linked PR, capping Python is entirely different. We're not going to be unaware of new major python releases, and users also have to explicitly choose to use a new Python instead of just having their dependency resolver select it in the background without them having any clue. Also I would be annoyed as a user to try out Dagster with new Python and have it install OK but then immediately fail. Very bad first experience.

👍

Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

OK, I'm sold.

Do you think we should try to identify specific packages that don't work on 3.12 yet? (duckdb immediately comes to mind). My take is that those are more likely to be fixed retroactively by upstream dependencies being upgraded without needing to update your dagster version

@smackesey
Copy link
Collaborator

Do you think we should try to identify specific packages that don't work on 3.12 yet

I don't think we should do so in any systematic way-- IMO it falls on the authors to bound their packages.

@rexledesma rexledesma force-pushed the rl/enforce-python-versions branch from 017e056 to 78bc476 Compare February 22, 2024 19:11
@rexledesma rexledesma merged commit b5bd636 into master Feb 22, 2024
1 check passed
@rexledesma rexledesma deleted the rl/enforce-python-versions branch February 22, 2024 20:15
PedramNavid pushed a commit that referenced this pull request Mar 28, 2024
## Summary & Motivation
Resolves #17933.

## How I Tested These Changes
- Create a virtualenv of Python 3.12, see installation fail.
- Create a virtualenv of Python 3.7, see installation fail.
- Create a virtualenv of Python 3.8, see installation succeed.
- See Buildkite still succeed.
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.

Fail early and explicitly on unsupported Python versions, like 3.12
4 participants