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

gh-468: fix virtualenv in testing #471

Merged
merged 17 commits into from
Jan 20, 2025
Merged

gh-468: fix virtualenv in testing #471

merged 17 commits into from
Jan 20, 2025

Conversation

paddyroddy
Copy link
Member

@paddyroddy paddyroddy commented Jan 13, 2025

Fixes #468, #479. The problem initially why the tests were failing was esheldon/fitsio#413 esheldon/fitsio#414. I have solved this by reinstating #396.

However, the tests were failing (just for 3.11) for a very niche nox bug wntrblm/nox#735. I'm still not particularly happy with nox, the ecosystem seems immature. I considered switching to tox #475, as I haven't had those issues before. However, this proved a bit tricky as I didn't want to change too much how we currently our running things, and we're relying on a fair bit of Python logic. In the end, this comment wntrblm/nox#735 (comment) referred to about modifying the cache and that seems to have solved the issue.

@paddyroddy paddyroddy added bug Something isn't working testing Work is related to testing labels Jan 13, 2025
@paddyroddy paddyroddy self-assigned this Jan 13, 2025
@paddyroddy paddyroddy marked this pull request as ready for review January 13, 2025 14:52
@paddyroddy paddyroddy changed the title Fix virtualenv in testing gh-468: fix virtualenv in testing Jan 13, 2025
@@ -7,8 +7,12 @@
import nox

# Options to modify nox behaviour
nox.options.default_venv_backend = "uv|virtualenv"
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed my issues locally using uv astral-sh/uv#6579

Copy link
Member

Choose a reason for hiding this comment

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

I remember we could not use uv because of astral-sh/uv#1794

I am surprised that it is working now. We still use the --prefer-binary flag in our tests -

--prefer-binary

Copy link
Member Author

Choose a reason for hiding this comment

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

But locally I'm not. With my uv environments locally it was failing without this. I'm not using the test-constraints.txt file.

Comment on lines 55 to 58
- name: Install ubuntu dependencies for fitsio
run: |-
sudo apt-get update
sudo apt-get install -y libbz2-dev
Copy link
Member Author

Choose a reason for hiding this comment

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

@paddyroddy paddyroddy marked this pull request as draft January 13, 2025 17:13
@paddyroddy paddyroddy marked this pull request as ready for review January 16, 2025 15:15
@@ -38,7 +38,7 @@ jobs:
- name: Cache nox
uses: actions/cache@v4
with:
key: test-${{ hashFiles('pyproject.toml') }}
key: test-${{ hashFiles('pyproject.toml') }}-${{ env.pythonLocation }}
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is weird. I have never encountered this bug before.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this, @paddyroddy! See my comments below -

@@ -38,7 +38,7 @@ jobs:
- name: Cache nox
uses: actions/cache@v4
with:
key: test-${{ hashFiles('pyproject.toml') }}
key: test-${{ hashFiles('pyproject.toml') }}-${{ env.pythonLocation }}
Copy link
Member

Choose a reason for hiding this comment

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

This is weird. I have never encountered this bug before.

@@ -166,7 +166,7 @@ def trapezoid_product(
y = np.interp(x, *f)
for f_ in ff:
y *= np.interp(x, *f_)
return np.atleast_1d(np.trapezoid(y, x, axis=axis))
return np.trapezoid(y, x, axis=axis) # type: ignore[no-any-return]
Copy link
Member

Choose a reason for hiding this comment

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

Should the mypy fixes be another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't asked for a review... The PR was open to get the tests running. I've merged in #484.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, my bad!

@@ -7,8 +7,12 @@
import nox

# Options to modify nox behaviour
nox.options.default_venv_backend = "uv|virtualenv"
Copy link
Member

Choose a reason for hiding this comment

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

I remember we could not use uv because of astral-sh/uv#1794

I am surprised that it is working now. We still use the --prefer-binary flag in our tests -

--prefer-binary

@paddyroddy paddyroddy mentioned this pull request Jan 20, 2025
@paddyroddy
Copy link
Member Author

Merging this, linting still failing.

@paddyroddy paddyroddy merged commit 29d3f5c into main Jan 20, 2025
14 of 15 checks passed
@paddyroddy paddyroddy deleted the paddy/issue-468 branch January 20, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing Work is related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python is not installed into the virtualenv in testing
2 participants