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

[BUG] Fix python version compatibility issues affecting tests #973

Merged

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented Nov 11, 2024

Fixes #972.

Description

This pull request:

  • Pins the pyyaml dependency to >6.0.1; without this, pyyaml is incompatible with cython>=3, a transitive dependency. We could have pinned pyyaml<=5.3.1, but those builds are quite old and I wasn't able to find solutions for all of our supported python versions.
  • Pins python to >=3.10 in environment-dev.yaml and pyproject.toml. This is not the default recommended python version specified in .python-version-default, but since this environment is used for testing, we unpin it here as well. Support for 3.8 and 3.9 is dropped.
  • Modifies the conda-store tests to actually run on the matrix of python versions (every run in the matrix was previously being done on .python-version-default!)
  • Modifies the conda-store-server integration tests to run on the matrix of supported versions.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit 16e196f
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/67353b9a7429320008a71cc8

@peytondmurray peytondmurray force-pushed the 972-fix-build-tests-issues branch 6 times, most recently from 7a931d2 to 0c866a4 Compare November 11, 2024 20:35
@peytondmurray peytondmurray requested a review from soapy1 November 11, 2024 20:37
Copy link
Contributor

@soapy1 soapy1 left a comment

Choose a reason for hiding this comment

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

could you also update the conda package recipe in this repo to reflect these updates

@@ -40,7 +40,7 @@ jobs:
- name: "Set up Python 🐍"
uses: actions/setup-python@v5
with:
python-version-file: .python-version-default
python-version: ${{ matrix.python-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Collaborator

@trallard trallard Nov 12, 2024

Choose a reason for hiding this comment

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

This only changes the Python version used within the GitHub action (to install hatch and run the lint, for which really we do not need to use a Python version matrix) rather than the version used for the tests since the runtime Python version for the latter comes from the Docker container used further down when doing docker compose up -d

I am unsure if that was the goal here, but it does not seem like it is.
To test against the various versions, we should be passing these as a build argument when building the Docker image (through docker compose ....)

@peytondmurray peytondmurray force-pushed the 972-fix-build-tests-issues branch 2 times, most recently from 717fa47 to 4a1db85 Compare November 11, 2024 21:27
recipe/meta.yaml Outdated
@@ -60,7 +60,7 @@ outputs:
- conda-store-worker = conda_store_server._internal.worker.__main__:main
requirements:
host:
- python >=3.8
- python 3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the motivation for pinning the python version to 3.8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, Jaime pinned us on the conda-store-feedstock, I think because CFEP-25 recommends testing against the oldest supported version. I've asked for clarification there, will report back here.

Comment on lines 91 to 98
# These mirror the tests run while building the conda package
# See https://github.com/conda-forge/conda-store-feedstock/ for details
- name: "Run basic import tests ✅"
run: |
python -c "import conda_store_server"
conda-store-server --help
conda-store-worker --help

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something must be different in the azure workflow environment because this check did not catch the issue we saw when trying to build the conda package. In any case I'll remove this now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we were not aligned with cep25 so we were building with 3.12 only and this did not show up

Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Would it not be best to drop 3.8 already per #974 rather than fixing now for Python 3.8?
This seems unnecessary for a version we need to drop anyway.

@trallard
Copy link
Collaborator

More comments

Pins python to >=3.8 in environment-dev.yaml to match our minimum version requirement specified in pyproject.toml. This is not the default recommended python version specified in .python-version-default, but since this environment is used for testing, we unpin it here as well.

Technically, this does not matter; the runtime Python version is/should be the one on the Docker container and/or the GH actions installed one.

Modifies the conda-store tests to actually run on the matrix of python versions (every run in the matrix was previously being done on .python-version-default!)
Modifies the conda-store-server integration tests to run on the matrix of supported versions.

This is actually needed, but please take a look at the comments above as the changes here do not actually achieve this goal. Also per #974 and my comment there I do not think it is worth the trouble of fixing compatibility with 3.8

@peytondmurray peytondmurray force-pushed the 972-fix-build-tests-issues branch from 3bef004 to 8157f0a Compare November 12, 2024 19:11
@peytondmurray peytondmurray force-pushed the 972-fix-build-tests-issues branch from 90334de to acb0beb Compare November 12, 2024 19:14
@peytondmurray
Copy link
Contributor Author

So while this seems to work locally with python_version=3.12 docker compose up --build, it looks like 3.10 is broken. The error suggests that it is something to do with the the version of packaging that is now "required" by setuptools>=70 (but not actually required, see pypa/setuptools#4478 for more context). The error originates in conda-lock which includes a vendored version of poetry which is calling out to virtualenv which in turn is calling out to setuptools. I tried pinning setuptools and packaging to the right versions but it hasn't made a difference here:

conda-store-server-1  | Traceback (most recent call last):
conda-store-server-1  |   File "<string>", line 1, in <module>
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/multiprocessing/spawn.py", line 116, in spawn_main
conda-store-server-1  |     exitcode = _main(fd, parent_sentinel)
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/multiprocessing/spawn.py", line 125, in _main
conda-store-server-1  |     prepare(preparation_data)
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/multiprocessing/spawn.py", line 236, in prepare
conda-store-server-1  |     _fixup_main_from_path(data['init_main_from_path'])
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/multiprocessing/spawn.py", line 287, in _fixup_main_from_path
conda-store-server-1  |     main_content = runpy.run_path(main_path,
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/runpy.py", line 289, in run_path
conda-store-server-1  |     return _run_module_code(code, init_globals, run_name,
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/runpy.py", line 96, in _run_module_code
conda-store-server-1  |     _run_code(code, mod_globals, init_globals,
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/runpy.py", line 86, in _run_code
conda-store-server-1  |     exec(code, run_globals)
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/bin/conda-store-server", line 5, in <module>
conda-store-server-1  |     from conda_store_server._internal.server.__main__ import main
conda-store-server-1  |   File "/opt/conda-store-server/conda_store_server/_internal/server/__main__.py", line 5, in <module>
conda-store-server-1  |     from conda_store_server._internal.server.app import CondaStoreServer
conda-store-server-1  |   File "/opt/conda-store-server/conda_store_server/_internal/server/app.py", line 35, in <module>
conda-store-server-1  |     from conda_store_server import __version__, storage
conda-store-server-1  |   File "/opt/conda-store-server/conda_store_server/storage.py", line 15, in <module>
conda-store-server-1  |     from conda_store_server import CONDA_STORE_DIR, api
conda-store-server-1  |   File "/opt/conda-store-server/conda_store_server/api.py", line 12, in <module>
conda-store-server-1  |     from conda_store_server._internal import conda_utils, orm, schema, utils
conda-store-server-1  |   File "/opt/conda-store-server/conda_store_server/_internal/orm.py", line 41, in <module>
conda-store-server-1  |     from conda_store_server._internal import conda_utils, schema, utils
conda-store-server-1  |   File "/opt/conda-store-server/conda_store_server/_internal/schema.py", line 13, in <module>
conda-store-server-1  |     from conda_lock.lockfile.v1.models import Lockfile
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/__init__.py", line 3, in <module>
conda-store-server-1  |     from conda_lock.conda_lock import main
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/conda_lock.py", line 50, in <module>
conda-store-server-1  |     from conda_lock.conda_solver import solve_conda
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/conda_solver.py", line 19, in <module>
conda-store-server-1  |     from conda_lock.interfaces.vendored_poetry import (
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/interfaces/vendored_poetry.py", line 9, in <module>
conda-store-server-1  |     from conda_lock._vendor.poetry.factory import Factory
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/_vendor/poetry/factory.py", line 18, in <module>
conda-store-server-1  |     from .repositories.pypi_repository import PyPiRepository
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/_vendor/poetry/repositories/pypi_repository.py", line 33, in <module>
conda-store-server-1  |     from ..inspection.info import PackageInfo
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/_vendor/poetry/inspection/info.py", line 25, in <module>
conda-store-server-1  |     from conda_lock._vendor.poetry.utils.env import EnvCommandError
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/conda_lock/_vendor/poetry/utils/env.py", line 23, in <module>
conda-store-server-1  |     import virtualenv
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/__init__.py", line 3, in <module>
conda-store-server-1  |     from .run import cli_run, session_via_cli
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/run/__init__.py", line 15, in <module>
conda-store-server-1  |     from .plugin.creators import CreatorSelector
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/run/plugin/creators.py", line 7, in <module>
conda-store-server-1  |     from virtualenv.create.via_global_ref.builtin.builtin_way import VirtualenvBuiltin
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/create/via_global_ref/builtin/builtin_way.py", line 5, in <module>
conda-store-server-1  |     from virtualenv.create.creator import Creator
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/create/creator.py", line 13, in <module>
conda-store-server-1  |     from virtualenv.discovery.cached_py_info import LogCmd
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/discovery/cached_py_info.py", line 25, in <module>
conda-store-server-1  |     _CACHE[Path(sys.executable)] = PythonInfo()
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/discovery/py_info.py", line 100, in __init__
conda-store-server-1  |     self.distutils_install = self._distutils_install().copy()
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/virtualenv/discovery/py_info.py", line 191, in _distutils_install
conda-store-server-1  |     i.finalize_options()
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/setuptools/command/install.py", line 67, in finalize_options
conda-store-server-1  |     super().finalize_options()
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/setuptools/_distutils/command/install.py", line 408, in finalize_options
conda-store-server-1  |     'dist_fullname': self.distribution.get_fullname(),
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/setuptools/_core_metadata.py", line 267, in get_fullname
conda-store-server-1  |     return _distribution_fullname(self.get_name(), self.get_version())
conda-store-server-1  |   File "/opt/conda/envs/conda-store-server/lib/python3.10/site-packages/setuptools/_core_metadata.py", line 285, in _distribution_fullname
conda-store-server-1  |     canonicalize_version(version, strip_trailing_zero=False),
conda-store-server-1  | TypeError: canonicalize_version() got an unexpected keyword argument 'strip_trailing_zero'

@peytondmurray
Copy link
Contributor Author

After execing into the container it looked like the canonicalize_version function at /opt/conda/envs/conda-store-server/lib/python3.10/site-packages/packaging/utils.py does indeed accept a strip_trailing_zeros argument so I'm not sure what's going on here. It's possible something is messing with sys.path to point the import system at some other vendored packaging.

@peytondmurray
Copy link
Contributor Author

I'm not sure why pinning minimum versions of setuptools and packaging didn't work, but pinning maximum versions did. Let's return to this once conda-lock is pluginized.

@peytondmurray peytondmurray force-pushed the 972-fix-build-tests-issues branch from c226752 to e00d5ba Compare November 12, 2024 23:13
@peytondmurray
Copy link
Contributor Author

@trallard Is there something that needs to be changed with the netlify configuration? I didn't expect these tasks to fail...

@trallard
Copy link
Collaborator

There were some issues with npm so kicked the preview build again and all is working now @peytondmurray

Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thanks @peytondmurray, this looks great 🚀
The only nit is that the server integration tests display in the UI is clunky as there is no quick way to see the Python version used (see screenshot below).
It might be worth updating the job name in .github/workflows/test_conda_store_server_integration.yaml (line 28) to something like `name: "integration-test - ${{ matrix.test-type }} (${{ matrix.python-version }})"

Sorry, GH does not allow to make suggestions on untouched lines 😬 I will approve this but would be great if we can update that name thing.

image

@peytondmurray
Copy link
Contributor Author

Thanks for the name change suggestion - it's way more readable this way. Will merge when tests pass!

@peytondmurray peytondmurray merged commit 3d6c97f into conda-incubator:main Nov 14, 2024
31 checks passed
@peytondmurray peytondmurray deleted the 972-fix-build-tests-issues branch November 14, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[BUG] - TypeAlias usage breaks conda-store-server for python <3.10
3 participants