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.

Sign up for GitHub

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

Jump to bottom

BLD: Test editable installations for PyWavelets in CI #702

Merged
merged 2 commits into from
Apr 18, 2024
Merged

BLD: Test editable installations for PyWavelets in CI #702

merged 2 commits into from
Apr 18, 2024

Conversation

agriyakhetarpal
Copy link
Collaborator

Closes #700. This PR introduces a flag in the Linux matrix setup to run an editable installation of PyWavelets and run the test suite with pytest.

I can rebase this branch and revisit this PR after mesonbuild/meson-python#569 is merged and meson-python==0.16.0 is out on PyPI – it shall fix the failing test as noted in #700 (comment).

rgommers
rgommers reviewed Feb 22, 2024
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@rgommers
Copy link
Member

Thanks! I'll get your other PR merged first, since there is some overlap. And then you can rebase this on top (CI will also run automatically once you have a PR merged).

@agriyakhetarpal
Copy link
Collaborator Author

Thanks! I rebased on top of the changes from the previous PR. The failure is expected, of course.

@agriyakhetarpal agriyakhetarpal force-pushed the editable-installations-ci branch 6 times, most recently from 6a0a43b to d06f627 Compare April 17, 2024 13:54
@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Apr 17, 2024 •
edited
Loading

With meson-python v0.16.0 out, this is now ready for review and the test suite against editable installations is working. It has been added for the Linux job for now – we can add it to the macOS job too if needed, but I think testing in just one place is perfectly fine as well. I rebased the branches and have revised the commit history, it should be easier to review/squash.

Edit: put out a request for review, didn't realise I forgot to do that :)

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review April 17, 2024 13:58
agriyakhetarpal
agriyakhetarpal commented Apr 17, 2024
View reviewed changes
.github/workflows/tests.yml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@agriyakhetarpal
Copy link
Collaborator Author

From the workflow run: https://github.com/PyWavelets/pywt/actions/runs/8723177022/job/23930800559?pr=702#step:4:446

this line shows that the wheel being built is an editable one, but the wheel does not have the editable keyword after the version and before the ABI tag – and I think this is a bug, it is not conformant with PEP 660. This is not the case with setuptools and other build-backends (just verified with both a Hatch-based pure Python project and a setuptools + pybind11 compiled one). I'll file this on the meson-python issue tracker.

@agriyakhetarpal agriyakhetarpal changed the title [WIP]: test editable installations for PyWavelets in CI BLD: Test editable installations for PyWavelets in CI Apr 17, 2024
@agriyakhetarpal agriyakhetarpal mentioned this pull request Apr 17, 2024
Closed
@rgommers
Copy link
Member

It has been added for the Linux job for now – we can add it to the macOS job too if needed, but I think testing in just one place is perfectly fine as well.

Agreed, a single job is fine.

rgommers
rgommers reviewed Apr 18, 2024
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Looks pretty good, only two minor comments.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@agriyakhetarpal
CI, BLD: Run tests on Linux editable installations
0ac654e 
This commit performs the following changes:

1. Adds a concurrency key to reduce duplicate runs
2. Moves common environment variables outside of
individual jobs and into the workflow context
for Linux and macOS
3. Adds an extra option to run an editable installation
of PyWavelets under the `--no-build-isolation` flag
with all of the build-time dependencies installed
beforehand, ignoring previous installations as
necessary.

The `pytest --pyargs pywt` invocation is used to
run the test suite against the editable installation
which is similar to the rest of the checks.
@agriyakhetarpal
BLD: Bump lower bound for meson-python
ae5de42 
Since the mechanism for editable installations
was improved in mesonbuild/meson-python#569
and subsequently released in meson-python
v0.16.0, this bumps the minimum version for it
for use during compilation.
@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Apr 18, 2024 •
edited
Loading

Thank you for the review, @rgommers! I have addressed all requested changes and rebased (only because they were minor). Let's hope that CI passes as usual.

rgommers
rgommers approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now, in it goes. Thanks @agriyakhetarpal, nice work!

@rgommers rgommers merged commit 96a65e1 into PyWavelets:main Apr 18, 2024
16 checks passed
@rgommers rgommers added this to the v1.7.0 milestone Apr 18, 2024
@agriyakhetarpal agriyakhetarpal deleted the editable-installations-ci branch April 18, 2024 19:31
@agriyakhetarpal agriyakhetarpal restored the editable-installations-ci branch May 3, 2024 21:33
@agriyakhetarpal agriyakhetarpal deleted the editable-installations-ci branch May 3, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgommers rgommers rgommers approved these changes

Assignees
No one assigned
Labels
testing
Projects
None yet
Milestone
v1.7.0
2 participants
@agriyakhetarpal @rgommers