Skip to content

Conversation

@sklarsa
Copy link

@sklarsa sklarsa commented Dec 11, 2025

Summary by CodeRabbit

  • Chores
    • Added CI coverage for macOS ARM (Apple Silicon), running tests across Python 3.10–3.13 to improve platform validation.
    • Added dedicated NumPy compatibility validation in the test pipeline, exercising multiple Python/NumPy combinations to ensure reliability across dependency versions.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 11, 2025

Walkthrough

Adds a new GitHub Actions workflow to run macOS ARM (Apple Silicon) tests with two jobs: a multi‑Python build/test job and a NumPy compatibility matrix test job targeting macOS ARM64 runners.

Changes

Cohort / File(s) Summary
CI workflow — macOS ARM tests
.github/workflows/test-macos-arm.yaml
New workflow defining two jobs: test (matrix Python 3.10–3.13 on macOS ARM64; steps: checkout, setup-python, install Rust, install Cython, install deps, proj.py build, proj.py test 1 with JAVA_HOME set) and test-numpy-compat (matrix of Python and NumPy versions; installs Rust and uv, runs tests pinned to specific NumPy versions targeting test/test.py -v TestBufferProtocolVersionV2).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check runner image and macOS/version label (ARM64 runner correctness)
  • Verify Python matrix entries and setup-python usage
  • Confirm Rust, Cython, and dependency install steps and order
  • Validate JAVA_HOME export and test invocation commands
  • Review NumPy compatibility matrix entries and targeted test path

Poem

🐰 I hopped into CI, swift and keen,
ARM64 leaves a shiny sheen,
Python versions, NumPy too,
Tests run through the morning dew —
A tiny rabbit cheers: “All green!” 🎉

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'add mac arm ci automation' directly and clearly describes the main change: adding CI automation for macOS ARM. It is concise, specific, and accurately reflects the primary purpose of the pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch steve-move-arm-ci

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8594de5 and b9cc326.

📒 Files selected for processing (1)
  • .github/workflows/test-macos-arm.yaml (1 hunks)
⏰ 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). (18)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel start_linux_arm64_agent_aws)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel windows_i686)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel linux_x64_cpython_manylinux_x86_64)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel linux_x64_pypy)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel macos_x64)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel windows_x86_64)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel linux_x64_cpython_musllinux)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on windows-msvc-2019)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on mac)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on linux-qdb-master)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on linux-old-pandas)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on linux)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing TestsAgainstVariousNumpyVersion1x)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing TestsAgainstVariousNumpyVersion2x)
  • GitHub Check: Test numpy compatibility (3.10, 1.26.0)
  • GitHub Check: Test on macOS ARM (3.12)
  • GitHub Check: Test numpy compatibility (3.10, 1.21.0)
  • GitHub Check: Test on macOS ARM (3.13)
🔇 Additional comments (2)
.github/workflows/test-macos-arm.yaml (2)

50-51: Remove the unused JAVA_HOME environment variable or correct the case sensitivity.

The environment variable JAVA_HOME_17_arm64 (lowercase) is available on macOS-14 ARM64 runners, but the workflow references it as JAVA_HOME_17_ARM64 (uppercase). On case-sensitive Unix/Linux systems, this mismatch causes the variable to expand to an empty string. Since Java is not used in the test execution, this environment variable assignment has no effect and should either be removed or corrected to the lowercase variant.

Likely an incorrect or invalid review comment.


53-93: Critical: Missing build step required for the extension module.

The numpy-compat job is missing essential setup steps that exist in the standard test job. When test/test.py runs, it will fail with an ImportError because the questdb.ingress Cython extension has never been compiled.

Add the following steps before running tests:

      - name: Install Cython
        run: |
          python3 --version
          python3 -m pip install --upgrade pip
          python3 -m pip install cython

      - name: Install test dependencies
        run: python3 ci/pip_install_deps.py

      - name: Build
        run: python3 proj.py build

Additionally, running only TestBufferProtocolVersionV2 may not be sufficient to catch numpy compatibility issues in other code paths. Consider whether all tests should run for each numpy version combination, or document why this single test class is sufficient for numpy validation.

⛔ Skipped due to learnings
Learnt from: amunra
Repo: questdb/py-questdb-client PR: 120
File: ci/run_tests_pipeline.yaml:85-85
Timestamp: 2025-11-28T17:46:34.703Z
Learning: In the py-questdb-client repository, Python version testing is handled by cibuildwheel configuration in pyproject.toml. The ci/run_tests_pipeline.yaml pipeline is specifically for testing against various numpy versions, not for comprehensive Python version coverage.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
.github/workflows/test-macos-arm.yaml (1)

31-40: Consider extracting Rust and toolchain setup to a reusable workflow.

Both jobs install Rust identically (lines 31–34 and 84–87). This duplication can be reduced by extracting the common setup steps into a reusable workflow or a composite action, improving maintainability.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3cd70bf and 8594de5.

📒 Files selected for processing (1)
  • .github/workflows/test-macos-arm.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: amunra
Repo: questdb/py-questdb-client PR: 120
File: ci/run_tests_pipeline.yaml:85-85
Timestamp: 2025-11-28T17:46:34.703Z
Learning: In the py-questdb-client repository, Python version testing is handled by cibuildwheel configuration in pyproject.toml. The ci/run_tests_pipeline.yaml pipeline is specifically for testing against various numpy versions, not for comprehensive Python version coverage.
⏰ 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). (15)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel windows_i686)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel windows_x86_64)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel macos_x64)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel linux_x64_cpython_musllinux)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel linux_x64_pypy)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel linux_x64_cpython_manylinux_x86_64)
  • GitHub Check: questdb.py-questdb-client-release (cibuildwheel start_linux_arm64_agent_aws)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on windows-msvc-2019)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on mac)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on linux-qdb-master)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on linux-old-pandas)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing on linux)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing TestsAgainstVariousNumpyVersion2x)
  • GitHub Check: questdb.py-questdb-client-tests (Building and testing TestsAgainstVariousNumpyVersion1x)
  • GitHub Check: Test numpy compatibility (3.11, 2.0.0)
🔇 Additional comments (4)
.github/workflows/test-macos-arm.yaml (4)

51-51: Verify that JAVA_HOME_17_ARM64 is available on macOS-13-xlarge ARM64 runners.

The environment variable JAVA_HOME_17_ARM64 may not be available on this runner. Please confirm it exists or consider using an alternative approach to set JAVA_HOME. You can check GitHub's documentation or test this in a workflow run to verify availability.


92-93: Verify the uv run --python python syntax is correct.

The command uv run --python python may not work as intended. The --python flag typically requires a specific path, version identifier, or interpreter name that uv can resolve. Verify that this selects the correct Python version or use an explicit path (e.g., --python python3.10).


92-93: The numpy compatibility test only runs a single test class.

The test command runs only TestBufferProtocolVersionV2 instead of the full test suite. If this is intentional for targeted numpy validation, this should be documented in a comment. If it's an oversight, consider running the full test suite with the numpy version matrix.


36-40: Inconsistent toolchain setup between jobs.

The test job installs Cython and uses python3 proj.py test, while test-numpy-compat skips Cython and uses uv run. Clarify whether Cython is required for the numpy compatibility tests or if the different invocation methods serve different purposes.

Also applies to: 89-93

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants