Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
| with: | ||
| target: x86_64 | ||
| args: --release --locked --out dist | ||
| args: --release --locked --out dist -i python3 |
There was a problem hiding this comment.
What changed and we need this?
There was a problem hiding this comment.
We had many interpreters available in the OS, we had to tell it specifically which one to use. It's complicated.
crates/tower/src/lib.rs
Outdated
|
|
||
| // Everything must happen inside block_on because Package holds a TmpDir | ||
| // whose Drop implementation requires an active tokio reactor. | ||
| rt.block_on(async { |
There was a problem hiding this comment.
nit: rt was not very descriptive here and I had to look for the declaration.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/build-binaries.yml:
- Line 109: Add the pip flag --no-index to every CI install invocation that
currently uses --find-links so pip won’t fallback to PyPI; update the
occurrences of "pip install ${{ env.PACKAGE_NAME }} --find-links dist/
--force-reinstall", the "python -m pip install" call, the standalone "pip
install" and "pip3 install" invocations, and the ".venv/bin/pip3 install"
commands to include --no-index (place it alongside --find-links/other flags) so
the tests only use the local dist artifacts.
- Around line 126-129: The workflow uses an invalid Maturin interpreter flag
value "interpreter: -i 3.13"; change that entry to provide an executable name
(e.g., "interpreter: -i python3" or "interpreter: -i python3.13") so the ARM
aarch64 build uses a valid --interpreter value; update the "interpreter: -i
3.13" line to match the other builds' "interpreter: -i python3" (or a specific
python executable) to avoid Maturin failing.
🧹 Nitpick comments (2)
src/tower/__init__.py (1)
28-29: Consider deferring native import failure for better UX.A guarded import would keep the package importable and surface a clearer error only when
build_packageis actually called.♻️ Suggested change
-from ._native import build_package +try: + from ._native import build_package +except Exception as exc: + def build_package(*args, **kwargs): + raise RuntimeError( + "tower._native extension is not available. Reinstall with a compatible wheel " + "or build from source with Rust." + ) from exctests/tower/test_build_package.py (1)
28-36: Close extracted file handles in_read_package.
tar.extractfile()returns a file-like object (io.BufferedReader) that should be closed to prevent file descriptor leaks when reading many entries.♻️ Suggested change
- if member.isfile(): - f = tar.extractfile(member) - entries[member.name] = f.read().decode("utf-8") if f else "" + if member.isfile(): + f = tar.extractfile(member) + if f: + with f: + entries[member.name] = f.read().decode("utf-8") + else: + entries[member.name] = ""
7f25bc6 to
bfe5e5c
Compare
This PR is our first bit of functionality exposing rust code to the Python SDK. It adds a new function
build_packagethat creates a package that can be used for deployments to Tower.Summary by CodeRabbit
Release Notes
New Features
build_package()function for programmatic use.Tests
Chores