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

Hatch to manage Python virtualenvs #87

Merged
merged 15 commits into from
Nov 8, 2022
Merged

Hatch to manage Python virtualenvs #87

merged 15 commits into from
Nov 8, 2022

Conversation

kafonek
Copy link
Contributor

@kafonek kafonek commented Nov 3, 2022

Alternative to, and closes #86 if you prefer this one.

Use hatch to manage Python virtualenv when developing instead of manually creating one or re-using a system Python.

This is what I'm seeing in the wheel metadata after building
image

Copy link
Collaborator

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

Thanks a lot @kafonek!
Some minor comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
```
pip install maturin
python -m hatch run maturin develop
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is a bit too much to always prepend python -m:

Suggested change
python -m hatch run maturin develop
hatch run maturin develop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you get a binary on your path when you installed hatch? I have Python 3.9.11 via pyenv as my default python (WSL OS). When I python -m pip install hatch, I can use python -m hatch afterwards but hatch is not found. I was going to actually drop a note on the hatch repo saying their docs seemed inconsistent with my experience.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, hatch is in my path. Maybe worth opening an issue there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I'll bring it up, switched the examples to use hatch and put a caveat next to the CONDA env caveat

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: David Brochart <[email protected]>
README.md Outdated Show resolved Hide resolved
Co-authored-by: David Brochart <[email protected]>
@davidbrochart
Copy link
Collaborator

I think we need to update the GitHub workflows.

@kafonek
Copy link
Contributor Author

kafonek commented Nov 3, 2022

I think we need to update the GitHub workflows.

Yep, up to you if you want it in this PR or a follow-up. I don't think I'll get to it until the weekend. 👍 from me if you want to merge this and update actions later, or if you want to edit this PR directly yourself, or just wait on it.

@davidbrochart
Copy link
Collaborator

I'm fine with waiting, there is no rush. That also lets time for other reviewers to chime in.

- name: Run Tests
run: |
pip install pytest
pytest
hatch run pytest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what happens if we try hatch run test:pytest here, presumably since there's only one version of Python on the runner then it's effectively the same as hatch run pytest.

@kafonek kafonek requested a review from davidbrochart November 6, 2022 00:43
README.md Outdated
Comment on lines 42 to 45
0. [Install Rust](https://www.rust-lang.org/tools/install) and [Python](https://www.python.org/downloads/)
- Some alternative bootstrapping options are [pyenv](https://github.com/pyenv/pyenv) for Python, or using [mamba](https://github.com/conda-forge/miniforge) to `mamba install rust python`.
1. Install [hatch](https://hatch.pypa.io/latest/install/) (`python -m pip install hatch`)
2. Create a development build of the library
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should remain agnostic to the users' package manager of choice in the setup. If we provide the default instructions with pip and have a pyproject.toml, more advanced users will be able to leverage it regardless of whether they are using poetry or hatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm good with whatever wording you want to use here. I think the original docs got across the point that you need bootstrapping before being able to build ypy, and these do as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for this section, lets keep it simple and use the pip instructions. We can keep the alternative bootstrapping section and link out to hatch docs in case people need extra assistance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure what tweaks you're asking for but I'm 👍 if you want to commit directly to the PR with the wording you'd like to see here. I think this may be the last item to resolve in the PR?

README.md Outdated
## Tests

All tests are located in `/tests`. You can run them with `pytest`.
All tests are located in `/tests`. There is a `test` environment matrix defined in `pyproject.toml` that will run `pytest` against `py37` through `py310`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is cool!

pyproject.toml Outdated
Comment on lines 19 to 20
[[tool.hatch.envs.test.matrix]]
python = ["37", "38", "39", "310"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to my comment above, lets keep the support for hatch within the build system. So this is great!

pyproject.toml Outdated Show resolved Hide resolved
@Waidhoferj
Copy link
Collaborator

I kept the hatch infrastructure, but reverted the README to pip-based directions

@Waidhoferj Waidhoferj merged commit 792dc96 into y-crdt:main Nov 8, 2022
@kafonek kafonek deleted the hatch branch November 8, 2022 02:32
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.

3 participants