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 1 commit
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
Copy link
Contributor

Choose a reason for hiding this comment

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

When I pip freeze with this locally, I get a lot more dependencies - should we be including those in this file too, or no? I don't know much about Python dependency management

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 believe those are installed from these and that doing it this way captures those too.
Although, I also don't know much about Python dependency management so I may be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can think of dependency management as applying additional constraints to a system.

Compare to sudoku. You have a bunch of empty squares (the version numbers for each dependency) and a bunch of numbers that can go in those squares (in this case, the list of all versions for a particular package). Then you have to find a set of numbers that go in those squares that satisfy the constraints. A general form of this is known as a "SAT solver" https://en.wikipedia.org/wiki/SAT_solver and there is a lot of academic work in trying to get SAT solvers to be as efficient as possible because they can be applied to a lot of sudoku-like search problems.

(The comparison becomes a lot more obvious when you use >= constraints rather than == since == is a lot more restrictive and plain-spoken about its intention.)

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, there are two points I want to make:

  1. I believe there's a push towards putting dependency constraints in pyproject.toml rather than requirements.txt. See for example https://github.com/Chia-Network/hsms/blob/main/pyproject.toml#L11
  2. maturin is a build-time dependency only. It does not need to be present when you're using the compiled wheel. So its version should really go in pyproject.toml (which is the "this is how to build me" file); see https://github.com/Chia-Network/chia_rs/blob/main/wheel/pyproject.toml#L2

packaging==23.2
pluggy==1.4.0
pytest==8.0.1
Loading