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

Use uv #504

Merged
merged 9 commits into from
Oct 15, 2024
Merged

Use uv #504

merged 9 commits into from
Oct 15, 2024

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Oct 9, 2024

Fixes #501.

To do:

@bemoody
Copy link
Collaborator

bemoody commented Oct 9, 2024

Please correct the issue link in the description above - this is issue #501, not #57.

It looks like the "uv pip install" command in run-tests.yml gives this error:

error: No virtual environment found; run `uv venv` to create an environment, or pass `--system` to install into a non-virtual environment

I'm not familiar with uv so I'm not sure exactly what this should be doing instead.

@bemoody
Copy link
Collaborator

bemoody commented Oct 9, 2024

As for hatchling versus poetry: a big difference is that hatchling includes the test suite and example data into the source distribution (wfdb-*-*-*.tar.gz). This makes the distribution 104 MB rather than 147 KB.

I'm not sure what to do. On the one hand, we probably ought to be including the test suite in the source distribution. On the other hand, the current test suite is vastly larger than it needs to be.

Also, since hatchling will include all files from the working directory (except those ignored by .gitignore?), it's important to run uv build from a pristine git clone or else run git clean -d -f -x beforehand. I know that's something I will forget to do, so please include that in the developers' notes.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 10, 2024

Regarding hatchling, you don't have to change the behavior if you don't want to, it just has different defaults. However, you can manually exclude tests and whatever else you don't want (by default, it ignores everything in .gitignore). Personally, I'd rather not include the tests if they significantly increase the size, which is clearly the case here.

@tompollard
Copy link
Member

Personally, I'd rather not include the tests if they significantly increase the size, which is clearly the case here.

I agree that it would be best to exclude tests from the distribution.

I haven't looked, but if doing this we should check where the examples in https://github.com/MIT-LCP/wfdb-python/blob/main/demo.ipynb are getting their data.

e.g. is record = wfdb.rdrecord('sample-data/a103l') getting the data from the build directory, or downloading it from somewhere?

The package has - or at least used to have - odd behavior around downloading data. It would go off to the internet to download data without being explicitly asked to do so.

@bemoody
Copy link
Collaborator

bemoody commented Oct 10, 2024

Let's not include the tests (or sample data or demos) in the sdist, then.

In my experience (having worked with a lot of different build systems, just not in Python) it is better to define what files should go into the package rather than what files shouldn't. I think it's probably best to do that by defining explicit "include" patterns in pyproject.toml.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 15, 2024

OK, so I had to bump Python to >= 3.8, because many packages are not available for 3.7 anymore. I suggest bumping to >= 3.9, because 3.8 is already EOL (https://devguide.python.org/versions/). Of course, support for 3.12 and 3.13 would be nice, but this requires NumPy >= 2, which will hopefully be supported soon.

I excluded the tests/ and sample-data/ directories as well as demo-img.png and demo.ipynb for the sdist, so it is now down to 155KB. If you prefer to use include patterns, please let me know what to include.

I've also removed the need to sync the version in two different places; now it is only available in wfdb/version.py.

I still have to update the developers guide and will include the note on the importance of cleaning the repository before building. I also have to find out how to set up API tokens for PyPI with uv.

@tompollard
Copy link
Member

This looks good to me.

I excluded the tests/ and sample-data/ directories as well as demo-img.png and demo.ipynb for the sdist, so it is now down to 155KB. If you prefer to use include patterns, please let me know what to include.

@bemoody my preference would be to merge this pull request, then deal with includes in a later change.

@tompollard tompollard merged commit 5427181 into MIT-LCP:main Oct 15, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot initialize project with poetry
3 participants