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

Add requirements.txt and update readme in wheel #488

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add requirements.txt and update readme in wheel #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f2ce89
add requirements.txt and update readme in wheel
matt-o-how Apr 25, 2024
a4a3f3a
change requirements to >=
matt-o-how Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions wheel/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,12 @@
The `chia_rs` wheel contains python bindings for code from the `chia` crate.

To run the tests:
```
cd wheel
python -m venv venv
. ./venv/bin/activate
python -m pip install -r requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider instead

  1. python -m pip install -e . (for a "development install" which points to the python files in the repo rather than making a pristine own copy that can let you "try out" python changes without reinstalling) or
  2. python -m pip install . (to make a pristine own copy that ignore further repo changes).

maturin develop
python -m pytest ../tests
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, when you're doing development, you probably do want maturin in your local venv so you can test rust changes by doing maturin develop rather than pip install -e . again, which takes longer because it creates a sandboxed venv to do the build.

So to do development, I guess ideally what you want

cd wheel
python -m venv venv
. ./venv/bin/activate
python -m pip install maturin==(whatever the version listed in pyproject.toml is)
python -m pip install -e '.[dev]'
maturin develop
python -m pytest ../tests

The .[dev] line means "please install additional package that this pyproject.toml file declares as being useful for development", so stuff like pytest, ruff, mypy, etc. We have to add stuff like in https://github.com/Chia-Network/hsms/blob/main/pyproject.toml#L19

Also, for obvious reasons, I'm not happy with the python -m pip install maturin==(whatever the version listed in pyproject.toml is) line. We could add maturin to the dev = ["maturin==...] line, but then we would have it in two places (the build section and the dev section) which is a recipe for disaster if they get out of sync and some incompatibility pops up and it works locally because we're using the dev version of maturin but not in CI because it uses the build version. I expect there is some pip thing to install the build tools, but I don't know what it is.

This comment is very dense with information, so feel free to ask for clarification if I'm not being clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the python dependencies are all '.[dev]' dependencies as the python is only used for running pytests

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a pyproject.toml file so there is a “python” project… it’s the wheel chia_rs. It happens to not have any python source code at the moment, but that’s okay. The CI still uses the pyproject.toml file to build it (or at least, it should!). The idea of pyproject.toml is to standardize how python-targeted wheels build lifecycles are specified, so IMO it's reasonable to add the dev dependencies here even if most developers end up using the maturin develop shortcut during their build/test development cycle.

Copy link
Contributor

@richardkiss richardkiss May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess you're talking about https://github.com/Chia-Network/chia_rs/blob/main/README.md#python-tests ?

Looks good. It might be helpful to note in the README that this mechanism to run the tests is a hack that is necessary because the chia_rs and chia_blockchain wheels currently have a cyclic dependency. At some point this can be fixed and then we'll be able to run the chia_rs tests with pip install .[tests] and pyproject.toml like I was hinting at above.

In any case, I think we can close this PR without merging it now. Thanks to @matt-o-how for bringing up this discussion. Prior to this, I hadn't internalized the cyclic dependency with chia-blockchain.

6 changes: 6 additions & 0 deletions wheel/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
iniconfig>=2.0.0
maturin>=1.4.0
chia-blockchain>=2.1.4
packaging>=23.2
pluggy>=1.4.0
pytest>=8.0.1
Loading