Sync v2 #3
Sync v2 #3
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughBump to v0.3.8; add explicit includes across C/C++ headers; update pybind11 and sse2neon submodule refs; update CI/CD GitHub Actions (poetry action, setup-python, cibuildwheel, artifact upload, expanded Python matrix, commented-out release jobs); update setup metadata and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Build Runner
participant CIBW as cibuildwheel
participant Artifact as Upload Artifact
rect rgba(200,230,255,0.4)
note right of GH: Trigger: push / pull_request
GH->>Runner: Setup environment (actions/setup-python@v5)
Runner->>Runner: Install Poetry (abatilo/actions-poetry@v3)
end
rect rgba(200,255,200,0.25)
note right of Runner: Build wheels phase
Runner->>CIBW: Run cibuildwheel (output: wheelhouse)\n(env: CIBW_PRERELEASE_PYTHONS=True, other CIBW_*)
CIBW-->>Runner: wheelhouse/
end
rect rgba(255,240,200,0.25)
note right of Runner: Artifact upload
Runner->>Artifact: actions/upload-artifact@v4 (upload: wheelhouse)
Artifact-->>GH: artifact stored
end
note over GH: Release jobs (release-pypi, release-github) are commented out in cd.yml
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
Comment |
mhaley-tignis
force-pushed
the
sync-v2
branch
3 times, most recently
from
e0426e7 to
c11b042
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/cd.yml (1)
95-97: Consider upgrading docker/setup-qemu-action to v3 or v4.Static analysis flagged
docker/setup-qemu-action@v2as outdated. While not part of this PR's explicit changes, upgrading to v3 or later would align with the overall modernization effort.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/cd.yml(5 hunks).github/workflows/ci.yml(2 hunks)CMakeLists.txt(1 hunks)include/streamvbyte.h(1 hunks)pybind11(1 hunks)pyproject.toml(1 hunks)setup.py(2 hunks)src/decode/streamvbyte_decode_scalar.hpp(1 hunks)src/decode/zigzag_decode_scalar.hpp(1 hunks)src/decode/zigzag_decode_vectorized.hpp(1 hunks)src/decode_shuffle_table.h(1 hunks)src/encode/streamvbyte_encode_scalar.hpp(1 hunks)src/encode/zigzag_encode_scalar.hpp(1 hunks)src/encode/zigzag_encode_vectorized.hpp(1 hunks)src/encode_shuffle_table.h(1 hunks)src/length_table.h(1 hunks)sse2neon(1 hunks)tests/test.py(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/cd.yml
98-98: the runner of "docker/setup-qemu-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build wheels of cp312-* windows-latest
- GitHub Check: Build wheels of cp310-* windows-latest
- GitHub Check: Build wheels of cp312-* ubuntu-latest
- GitHub Check: Build wheels of cp313-* windows-latest
- GitHub Check: Build wheels of cp311-* windows-latest
- GitHub Check: Build wheels of cp311-* ubuntu-latest
- GitHub Check: Build wheels of cp310-* ubuntu-latest
- GitHub Check: Build wheels of cp313-* ubuntu-latest
🔇 Additional comments (27)
src/decode/streamvbyte_decode_scalar.hpp (1)
7-7: LGTM! Good defensive practice.Adding the explicit
<cstdint>include ensures fixed-width integer types (uint32_t,uint8_t, etc.) are available without relying on transitive includes, improving portability.src/decode/zigzag_decode_scalar.hpp (1)
7-7: LGTM! Consistent with the broader pattern.This explicit include aligns with the same change across other headers and ensures
uint32_tandint32_tare properly declared.src/encode/zigzag_encode_scalar.hpp (1)
7-7: LGTM! Maintains consistency.The explicit
<cstdint>include follows the same pattern applied across encode/decode headers and properly supports the fixed-width integer types used in this file.pyproject.toml (1)
3-3: LGTM! Version bump is aligned.The version increment to 0.3.8 is consistent with the broader project release updates mentioned in the PR summary.
src/encode/zigzag_encode_vectorized.hpp (1)
7-7: LGTM! Completes the consistent include pattern.Adding
<cstdint>ensures the fixed-width integer types are available for both scalar operations and the vectorized SIMD code paths.sse2neon (1)
1-1: Routine submodule update for sse2neon dependency.This is a forward update of the sse2neon submodule pointer to a newer commit that fixes test failures with -O2 optimization flags. As a header-only library, this update is safe and carries no compatibility risks with libstreamvbyte.
src/decode_shuffle_table.h (1)
4-4: Correct addition of required standard header.Adding
#include <cstdint>is necessary to provide theuint8_ttype definition used in thedecode_shffule_tablearray declaration at line 6. This aligns with the PR's systematic addition of explicit<cstdint>includes across C/C++ headers for better portability and compliance with modern C++ practices.pybind11 (1)
1-1: I've confirmed the new pybind11 commit is legitimate. Now I need to check for potential breaking changes between the old and new commits.Pybind11 submodule update is legitimate. Verify test results with updated code.
The new commit (8e7307f0699726c29f5bdc2c177dd55b6b330061) is from July 24, 2024 and is a documentation fix. The commit is a legitimate update from the official pybind11 repository.
The PR's addition of explicit
<cstdint>headers aligns with pybind11 best practices; earlier pybind11 versions removed#include <iostream>from stl.h, requiring projects to use "Include What You Use" to add necessary headers explicitly.Ensure all tests pass with the updated submodule before merging.
src/length_table.h (1)
4-4: LGTM! Portability improvement.Explicitly including
<cstdint>ensuresuint8_tis available, improving portability across different compilers and platforms.src/encode/streamvbyte_encode_scalar.hpp (1)
7-7: LGTM! Portability improvement.Explicitly including
<cstdint>ensures fixed-width integer types (uint8_t,uint32_t) are available.src/encode_shuffle_table.h (1)
4-4: LGTM! Portability improvement.Explicitly including
<cstdint>ensuresuint8_tis available for the shuffle table declaration.include/streamvbyte.h (1)
5-5: LGTM! Essential for public API portability.Explicitly including
<cstdint>in the public header ensures fixed-width integer types used throughout the API (uint8_t,uint32_t,int32_t) are properly defined.CMakeLists.txt (1)
2-2: LGTM! Version bump is consistent.The version bump to 0.3.8 aligns with the version updates in other build configuration files.
.github/workflows/ci.yml (2)
67-67: LGTM! Base image format update.The change from
.7zto.img.xzis simply an archive format update for the same DietPi ARM base image.
29-31: Poetry 1.8.2 is compatible with this project.The abatilo/actions-poetry@v3 action is actively tested with Poetry 1.8.4, confirming that v3 works properly with the 1.8.x minor version line. The most significant evidence of compatibility is that the poetry.lock file was already generated by Poetry 1.8.2.
The project's pyproject.toml specifies python = "^3.8", which is compatible with Poetry 1.8.2. The dependencies are straightforward—setuptools ^65.6.3 for the main build and pytest/numpy for testing—with no special configurations that would break. The transition from Poetry 1.2.2 to 1.8.2 stays within the 1.x line, so no major version breaking changes apply.
src/decode/zigzag_decode_vectorized.hpp (1)
7-7: LGTM! Portability improvement.Explicitly including
<cstdint>ensures fixed-width integer types (uint32_t,int32_t) are available for the zigzag decode function.setup.py (2)
99-99: LGTM! Version bump is consistent.The version bump to 0.3.8 aligns with the version updates in CMakeLists.txt and other configuration files.
123-124: Verify Python 3.12 and 3.13 support.The addition of Python 3.12 and 3.13 classifiers indicates support for these versions. Ensure that the C++ extension builds and tests pass on these Python versions, especially since the CI configuration only tests Python 3.10.4.
Consider expanding the CI matrix to include Python 3.12 and 3.13 if these versions are now officially supported:
# Example addition to .github/workflows/ci.yml strategy: matrix: python-version: ["3.10", "3.11", "3.12", "3.13"]tests/test.py (1)
57-57: Version assertion aligns with release bump.Line 57 correctly reflects the v0.3.8 release documented in the PR objectives and broader version updates across the codebase.
.github/workflows/cd.yml (8)
4-4: Enable PR-triggered CD pipeline.Adding the
pull_requesttrigger allows the full build and test suite to run on pull requests, enabling early validation before merge.
34-36: Poetry action and version update.The switch to
abatilo/actions-poetry@v3with poetry 1.8.2 modernizes dependency management in the workflow using a well-maintained community action.
72-74: Update to actions/upload-artifact@v4.Upgrading from v3 to v4 aligns with current GitHub Actions best practices and resolves deprecation warnings.
84-84: Expand Python version matrix for future compatibility.Adding cp313 and cp314 extends wheel building to Python 3.13 and the pre-release Python 3.14, with pre-release handling controlled by the
CIBW_PRERELEASE_PYTHONSenvironment variable set at line 110.
93-93: Update to actions/setup-python@v5.Upgrading from v3 to v5 brings modern Python tooling and eliminates outdated dependencies.
99-110: Adopt cibuildwheel for standardized wheel building.Introducing explicit
cibuildwheel2.19.2 with dedicated build step modernizes the wheel-building pipeline. TheCIBW_PRERELEASE_PYTHONS: Truesetting enables experimental versions like Python 3.14.
112-114: Consistent artifact upload action.Using
actions/upload-artifact@v4aligns with the v4 upgrade at line 72.
116-166: Release jobs intentionally commented out.The PyPI and GitHub release workflows are preserved in commented form, suggesting these are temporarily disabled pending other synchronization work. Ensure this aligns with the PR's release readiness before merge.
mhaley-tignis
force-pushed
the
sync-v2
branch
2 times, most recently
from
10c4403 to
301939c
Compare
mhaley-tignis
force-pushed
the
sync-v2
branch
from
301939c to
2e30336
Compare
They are failing, and we don't need them. Easiest to remove.
No description provided.