search

gauge-sh / tach

A Python tool to enforce dependencies, using modular architecture 🌎 Open source 🐍 Installable via pip 🔧 Able to be adopted incrementally - ⚡ Implemented with no runtime impact ♾️ Interoperable with your existing systems 🦀 Written in rust
https://gauge.sh
MIT License
953 stars 33 forks source link

Bump pyo3 #153

Closed clin1234 closed 1 month ago

clin1234 commented 1 month ago

Fixes #152

emdoyle commented 1 month ago

@clin1234 Your changes look good, but if I'm reading the failure output correctly, it looks like pydantic-core==2.18.2 doesn't distribute a pre-built wheel for 3.13, and it needs an incompatible version of pyo3 (0.21.1). This requirement must be coming from pydantic which we have as a floating version.

I would recommend upping our pydantic version requirement when python_version is 3.13, since the latest version of pydantic and pydantic-core seem to be using a later version of pyo3.

clin1234 commented 1 month ago

I do not know where that specific version of pydantic-core is pulled from: latest release is https://github.com/pydantic/pydantic-core/releases

clin1234 commented 1 month ago

Perhaps you should setup dependabot support so that manual updates to dependencies won't be necessary...

emdoyle commented 1 month ago

Perhaps you should setup dependabot support so that manual updates to dependencies won't be necessary...

We may in the future, but the change you are trying to make here is purely to support Python 3.13, which is a beta release and not a target version that Tach supports today, so I don't think Dependabot would have changed anything here.

Your latest commit updates the dev requirements, but the issue here is the package requirement in pyproject.toml: "pydantic~=2.0"

This dependency is purposefully loose so that we can avoid unnecessary conflicts in users' Python projects, since pydantic is a very common dependency. Upgrading it to force a late enough version of pydantic-core to support the beta release of Python 3.13 is not a good tradeoff in general, so the only reasonable change I could see here is a specific version upgrade when the detected python version is 3.13 or higher. You can see an example of using the python version to specify dependencies also in pyproject.toml:

    "stdlib-list>=0.10.0; python_version < '3.10'",
    "eval-type-backport>=0.2.0; python_version < '3.10'"

If you can specify a workable version range for Python 3.13 and avoid impacting other users, that would work.

clin1234 commented 1 month ago

Perhaps you should setup dependabot support so that manual updates to dependencies won't be necessary...

We may in the future, but the change you are trying to make here is purely to support Python 3.13, which is a beta release and not a target version that Tach supports today, so I don't think Dependabot would have changed anything here.

Your latest commit updates the dev requirements, but the issue here is the package requirement in pyproject.toml: "pydantic~=2.0"

This dependency is purposefully loose so that we can avoid unnecessary conflicts in users' Python projects, since pydantic is a very common dependency. Upgrading it to force a late enough version of pydantic-core to support the beta release of Python 3.13 is not a good tradeoff in general, so the only reasonable change I could see here is a specific version upgrade when the detected python version is 3.13 or higher. You can see an example of using the python version to specify dependencies also in pyproject.toml:

    "stdlib-list>=0.10.0; python_version < '3.10'",
    "eval-type-backport>=0.2.0; python_version < '3.10'"

If you can specify a workable version range for Python 3.13 and avoid impacting other users, that would work.

I haven't modified pyproject.toml, where most users would find a suitable version of pydantic; I only bumped the version of pydantic within the dev requirements, in order to fix a specific dep for building on 3.13 through CI. This shouldn't impact the workflow of a casual user.

emdoyle commented 1 month ago

I haven't modified pyproject.toml, where most users would find a suitable version of pydantic; I only bumped the version of pydantic within the dev requirements, in order to fix a specific dep for building on 3.13 through CI. This shouldn't impact the workflow of a casual user.

I see what you're saying - all Rust dependencies should be build-time/dev dependencies assuming we pre-build wheels for all supported environments. In that case, there may be another change required to the publish workflow. It uses the --find-interpreter option to determine which Python interpreters it will build wheels for. This depends on the interpreters installed in our GitHub Actions runner, which I believe are shown here: https://github.com/gauge-sh/tach/actions/runs/9848383661/job/27190215219#step:4:394

This means we will not build wheels for 3.13, so if you are planning to use this version you'll need to manage the Rust toolchain on your local machine as well - or you'll need to modify the publish.yml to setup Python 3.13 (and also it looks like we are missing 3.7)

I'm happy to merge this as-is and deal with the wheel change separately, or if you want to make the GHA change and use this branch that's fine too!

clin1234 commented 1 month ago

Good enough. I'll make the publish.yml change in a different branch