-
Notifications
You must be signed in to change notification settings - Fork 50
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
[BUG] Fix python version compatibility issues affecting tests #973
Conversation
✅ Deploy Preview for conda-store canceled.
|
7a931d2
to
0c866a4
Compare
There was a problem hiding this 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 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
There was a problem hiding this comment.
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 ....
)
717fa47
to
4a1db85
Compare
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
# 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a workflow that tests the conda build at https://github.com/conda-incubator/conda-store/blob/main/.github/workflows/conda.yml
Ref run: https://github.com/conda-incubator/conda-store/actions/runs/11789976339/job/32839661575?pr=975
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
More comments
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.
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 |
3bef004
to
8157f0a
Compare
90334de
to
acb0beb
Compare
So while this seems to work locally with
|
After execing into the container it looked like the |
I'm not sure why pinning minimum versions of |
c226752
to
e00d5ba
Compare
@trallard Is there something that needs to be changed with the netlify configuration? I didn't expect these tasks to fail... |
There were some issues with npm so kicked the preview build again and all is working now @peytondmurray |
There was a problem hiding this 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.
Thanks for the name change suggestion - it's way more readable this way. Will merge when tests pass! |
Fixes #972.
Description
This pull request:
pyyaml
dependency to>6.0.1
; without this,pyyaml
is incompatible withcython>=3
, a transitive dependency. We could have pinnedpyyaml<=5.3.1
, but those builds are quite old and I wasn't able to find solutions for all of our supported python versions.>=3.10
inenvironment-dev.yaml
andpyproject.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.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
!)conda-store-server
integration tests to run on the matrix of supported versions.Pull request checklist