refactor(python): readme improvements - #3825
Conversation
… workflow - small fixes
|
/ready |
|
@ethanlin01x @jiengup Hi guys, it is optional but would be helpful if you test these instructions on your machine and see whether it works. @ethanlin01x you mentioned you're on MacOS and you're willing to test it, your input on what could be added here will be valuable. Thanks. |
| > Install the dependencies with `pip`: | ||
| > | ||
| > ```bash | ||
| > pip install -e ".[all]" |
There was a problem hiding this comment.
pip path lost its rebuild step - master had maturin develop after pip install -e ".[all]", this rewrite dropped it. there's no pure-python source here; the module is the compiled cdylib, and the editable install copies the built .so into site-packages (direct_url.json says editable, but there's no .pth redirect and no import hook - the artifact is a snapshot). so after any rust edit, pytest silently tests the old binary. a new symbol at least fails loud with AttributeError, but changed behavior on an existing method just runs stale code, and the fix-a-bug-then-retest loop is actively misleading - the test keeps failing and you start "fixing" code that was already correct. re-add an explicit rebuild command to the pip path.
| 2. Run the server to be able to run the tests | ||
|
|
||
| ```bash | ||
| cargo run --bin iggy-server -- --with-default-root-credentials --fresh |
There was a problem hiding this comment.
step 2 only works from repo root - foreign/python is excluded from the root workspace, so from the sdk dir cargo errors with no bin target named 'iggy-server'. meanwhile steps 1/3/4 only work from foreign/python. no step says where to run it from, so the numbered sequence can't be followed from any single directory - a per-command cwd note would fix the whole thing. two more small things here: the server runs foreground (steps 3-5 need a second terminal), and --fresh wipes local_data/ on every start (the flag's own help says "THIS WILL DELETE ALL DATA!") - worth a word of warning now that it's the only documented way to start the server.
| > Build the project - this runs cargo build and performs an editable install: | ||
| > | ||
| > ```bash | ||
| > uv run maturin develop |
There was a problem hiding this comment.
missing --no-sync. bare uv run implicitly syncs, which builds and installs the project via the pep517 backend (release profile) right before maturin develop builds it again in debug - two full cargo builds per iteration, one thrown away. it also undoes the --no-install-project added on line 55, so that flag currently buys nothing. line 95 already does this correctly, and CI uses uv run --no-sync maturin develop in .github/actions/python-maturin/pre-merge/action.yml.
|
|
||
| ```bash | ||
| # Using uv | ||
| uv venv # if not already created |
There was a problem hiding this comment.
uv venv isn't the missing prerequisite here - in a bare directory uv add apache-iggy fails with No pyproject.toml found in current directory or any parent directory, venv or not (uv add needs a project). since this readme is also the pypi landing page, better to show uv pip install apache-iggy for the standalone flow and drop this line. uv add is fine when the reader already has a project.
| 5. Before committing, test the pre-commit and pre-push hooks. `prek` only inspects staged content, so stage your work first: | ||
|
|
||
| ```bash | ||
| git add -A |
There was a problem hiding this comment.
this sequence wedges when any fixer hook modifies a file: fixes land in the worktree, the index keeps the unfixed blob, prek run exits 1, and the pre-push run just stashes the fix and re-checks the stale index (Hook changes conflicted with the saved unstaged changes. Reverting the hook changes) - zero progress until you re-stage. add a note to re-run git add -A after any run where hooks changed files. (prek run --all-files is not a substitute - it reads the worktree and passes while the staged blob is still broken, and it feeds the whole repo to the typos -w fixer.)
| maturin develop | ||
| pytest tests/ -v # Run tests (requires iggy-server running) | ||
| ``` | ||
| 1. Build a project for development |
There was a problem hiding this comment.
prerequisites are understated - the only stated requirement in this file is python 3.10+, but every path needs a rust toolchain from step 1 onward (this step literally runs cargo build, and the pip variant compiles the extension too), plus uv and prek. and since the docker option was dropped, there's no toolchain-free way to get a server anymore. a short prerequisites line plus a pointer to the prek install section of the root CONTRIBUTING.md would help.
| 4. To update the stubs, use | ||
|
|
||
| ```bash | ||
| cargo run --bin stub_gen |
There was a problem hiding this comment.
two things: the command only resolves from foreign/python (the crate is not in the root workspace), and it's worth saying when to run it - only after changing the pyo3 surface; nothing in CI checks stub freshness, so unconditional regen just invites .pyi churn. separate but related: stub_gen.rs builds its license-prepend path from file!(), which resolves against the runtime cwd - run this from any subdirectory of foreign/python and it rewrites the tracked apache_iggy.pyi without the apache header before failing with a bare ENOENT. the CI license gate catches it, but Path::new(env!("CARGO_MANIFEST_DIR")) in that binary would make step 4 cwd-independent. happy to see that land separately since the code is outside this diff.
|
|
||
| With `uv`: | ||
|
|
||
| > Create a venv: |
There was a problem hiding this comment.
style: blockquote-wrapped code fences are new to this repo - every other readme (and steps 2-4 below) uses plain fences, and > elsewhere means the ASF disclaimer. collapsing each install path to one fence with # comments reads the same and drops ~40 lines.
| ## Contributing | ||
|
|
||
| See [CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/foreign/python/CONTRIBUTING.md) for development setup and guidelines. | ||
| See [CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/CONTRIBUTING.md) for development setup and guidelines. |
There was a problem hiding this comment.
the retarget itself is right (foreign/python/CONTRIBUTING.md doesn't exist, so the old link was a 404), but the root CONTRIBUTING.md delegates language-specific setup back to this very readme, so "for development setup" now sends the reader in a circle. "for contribution guidelines" fits better.
| prek run --hook-stage pre-push | ||
| ``` | ||
|
|
||
| These are some of the essential commands prek is running, so it's recommended to run them manually before running prek / committing / pushing. This list is not exhaustive and other hook failures are possible. |
There was a problem hiding this comment.
double space in "before running". this line is also 212 chars - worth wrapping.
github-actions
Bot
added
S-waiting-on-author
| `uv`: | ||
|
|
||
| ```bash | ||
| uv run --no-sync pytest tests/ -v |
There was a problem hiding this comment.
step 3 does not mention docker. not asking for the compose file back - #3750 defers that - but tests/test_tls.py needs a daemon regardless: it starts apache/iggy:edge through testcontainers with no skip guard, so all 6 tests there error at fixture setup without one. --all-extras on line 55 pulls testcontainers in, so this is the default path.
worth listing docker under prerequisites, or giving the no-docker variant uv run --no-sync pytest tests/ -v --ignore=tests/test_tls.py. same for line 101.
|
|
||
| ```bash | ||
| git add -A | ||
| prek run # runs pre-commit hooks |
There was a problem hiding this comment.
same --no-sync issue as line 61, but in the hook and with the opposite outcome. .pre-commit-config.yaml:45 runs uv run pyrefly check with no --no-sync, on any staged foreign/python/**.{py,pyi} - which step 4's regenerated .pyi plus the git add -A above makes routine. on line 61 the sync lands before maturin develop, so the release build is the one thrown away and you still end up with debug. here it lands after step 1, so the release build wins and installs over the debug .so you were testing against.
the real fix is in the hook, which is outside this diff and wants its own pr. for now a note under step 5 would do: "prek run reinstalls the package as a side effect, re-run uv run --no-sync maturin develop before going back to step 3" - removable once the hook is fixed. happy to open an issue for it.
3 participants
Which issue does this PR address?
Closes #3750
Rationale
The documentation provided in Python SDK does not provide a golden path for development workflow. The absence of golden path adds the cognitive load on most devs, especially the new ones.
What changed?
README for Python SDK now provides a clear instruction on how to build and test it and gives basic advice on troubleshooting for precommit and prepush hooks. The new path is tested by hand and has fixed a number of unwanted side effects described in the linked issue.
Local Execution
AI Usage
If AI tools were used, please answer: