feat(python bindings): enable python bindings and example#307
Merged
Conversation
github-actions
Bot
added
c/cpp
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds native Python bindings (pybind11) and example usage, updates build and dev container to install Python tooling, enables PIC for C targets, and fixes an internal CIELAB→RGB assignment bug. Changes
Sequence Diagram(s)sequenceDiagram
participant Py as Python script
participant NumPy as NumPy ndarray
participant Module as pybind11 img2num
participant Lib as Img2Num C library
participant CAlloc as C allocator
Py->>NumPy: prepare RGBA ndarray
Py->>Module: call bilateral_filter(ndarray, w, h, ...)
Module->>NumPy: get writable data pointer
Module->>Lib: img2num_bilateral_filter(ptr, w, h, ...)
Lib-->>Module: in-place modified buffer
Module-->>Py: returns None (in-place)
Py->>Module: call kmeans(ndarray, w, h, k, ...)
Module->>NumPy: read input pointer
Module->>Lib: img2num_kmeans(in_ptr, out_ptr, labels_ptr, ...)
Lib-->>Module: fills out buffers
Module-->>Py: return (out_image_ndarray, labels_ndarray)
Py->>Module: call labels_to_svg(data, labels, w, h, min_area)
Module->>Lib: img2num_labels_to_svg(...)
Lib-->>Module: returns char* svg_cstr
Module->>CAlloc: free(svg_cstr)
Module-->>Py: return svg_str
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
core/CMakeLists.txt (1)
22-25: Debug messages may print empty values and use inconsistent log level.
Python3_LIBRARIESandPython3_INCLUDE_DIRSare only populated whenfind_package(Python3)includes theDevelopmentcomponent. Since line 21 only requestsInterpreter, these variables will likely be empty. Also, these messages lack theSTATUSkeyword used elsewhere in this file.Consider removing these debug messages or using
STATUSfor consistency:♻️ Suggested fix
find_package(Python3 COMPONENTS Interpreter REQUIRED) -message("Python Version: ${Python3_VERSION}") -message("Python Version: ${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR}") -message("Python Library: ${Python3_LIBRARIES}") -message("Python Include: ${Python3_INCLUDE_DIRS}") +message(STATUS "Python Version: ${Python3_VERSION}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/CMakeLists.txt` around lines 22 - 25, The current debug messages use message() without STATUS and print variables (Python3_LIBRARIES, Python3_INCLUDE_DIRS) that will be empty because find_package(Python3) requested only the Interpreter component; update the messages to either remove the two library/include lines or change find_package(Python3) to request the Development component if you need them, and ensure all message calls use message(STATUS ...) for consistency with other logs (refer to the find_package(Python3) invocation and the message(...) lines in this diff).docker-compose.yml (1)
7-7: Reminder: Update the Docker Hub image.Commenting out the image line forces a local build from
Dockerfile.dev. Per the PR description, the Docker container on Docker Hub needs to be updated with the new Python components. Once updated, this line should be uncommented to restore the cached image workflow.Would you like me to open an issue to track updating the Docker Hub image (
ryanmillard/img2num-dev:latest)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 7, Uncomment and restore the Docker Hub image line in docker-compose.yml so the service uses ryanmillard/img2num-dev:latest (or a new versioned tag) instead of forcing a local build; then rebuild and push an updated Docker Hub image that includes the new Python components (tag it appropriately, e.g. :latest or a semver tag) and update the image reference if needed so the compose file pulls the updated image rather than building from Dockerfile.dev.example-apps/console-py/main.py (1)
21-28: Consider named constants for algorithm parameters.Magic numbers make it harder to understand what each parameter controls. For an example script, adding comments or constants improves learnability:
📖 Suggested improvement
+# Bilateral filter parameters +SIGMA_SPATIAL = 3 +SIGMA_RANGE = 50 +COLOR_SPACE_RGB = 0 + # bilateral filter in-place -img2num.bilateral_filter(img, width, height, 3, 50, 0) +img2num.bilateral_filter(img, width, height, SIGMA_SPATIAL, SIGMA_RANGE, COLOR_SPACE_RGB)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/console-py/main.py` around lines 21 - 28, Replace the numeric literals passed into img2num.bilateral_filter, img2num.kmeans, and img2num.labels_to_svg with named constants (e.g., BILATERAL_DIAMETER, BILATERAL_SIGMA_COLOR, BILATERAL_SIGMA_SPACE, KMEANS_CLUSTERS, KMEANS_ITERATIONS, KMEANS_ATTEMPTS, SVG_SCALE) and add short inline comments where those constants are declared to explain their meaning; update calls in the main function (the usages of img2num.bilateral_filter, img2num.kmeans, and img2num.labels_to_svg) to use those constant names instead of magic numbers so the parameters are self-describing and easy to tweak for examples.Dockerfile.dev (1)
62-64: Consider combining pip installs and pinning versions.Two separate
RUNcommands create unnecessary layers. Combining them and pinning versions improves build efficiency and reproducibility:♻️ Suggested fix
# Install some pip packages -RUN python3 -m pip install numpy --break-system-packages -RUN python3 -m pip install opencv-python --break-system-packages +RUN python3 -m pip install --break-system-packages \ + numpy==2.0.0 \ + opencv-python==4.10.0.84🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.dev` around lines 62 - 64, Combine the two separate RUN pip install lines into a single RUN to avoid extra image layers and pin package versions for reproducibility; replace the two RUN lines that call "python3 -m pip install numpy --break-system-packages" and "python3 -m pip install opencv-python --break-system-packages" with one RUN that installs both packages with explicit version pins (e.g., numpy==<version> opencv-python==<version>) and include flags like --no-cache-dir (or use a requirements.txt/constraints file) while preserving --break-system-packages as needed.CMakeLists.txt (1)
30-32: Consider making Python bindings optional.Python bindings are built unconditionally for non-Emscripten builds. Users who don't need Python support may prefer to skip this (especially if
pybind11is not installed). Consider adding an option:♻️ Suggested approach
+option(IMG2NUM_BUILD_PYTHON_BINDINGS "Build Python bindings" ON) + if(EMSCRIPTEN) # JS bindings add_subdirectory(bindings/js) else() - # Python bindings - add_subdirectory(bindings/python) + if(IMG2NUM_BUILD_PYTHON_BINDINGS) + # Python bindings + add_subdirectory(bindings/python) + endif() if(IMG2NUM_BUILD_EXAMPLES)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 30 - 32, Add a CMake option to make building Python bindings conditional: introduce an option like BUILD_PYTHON_BINDINGS (default ON for non-Emscripten) and wrap the existing add_subdirectory(bindings/python) call in an if(BUILD_PYTHON_BINDINGS) ... endif() block; also ensure the option defaults to OFF for Emscripten builds (detect via CMAKE_SYSTEM_NAME or EMSCRIPTEN variable) and update any README or top-level help text accordingly so users without pybind11 can skip the Python bindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/CMakeLists.txt`:
- Around line 11-12: Remove the deprecated find_package(PythonLibs REQUIRED)
call and the hardcoded PYTHON_MODULE_EXTENSION set (".so"); instead rely on the
existing find_package(Python3 COMPONENTS Development) already used and let
pybind11_add_module pick the correct extension for each platform. Edit the
CMakeLists to delete references to find_package(PythonLibs) and remove or stop
using the PYTHON_MODULE_EXTENSION variable so the module build uses
Python3/pybind11 defaults.
In `@bindings/python/src/img2num_pybind.cpp`:
- Around line 46-67: Add input validation at the start of the kmeans binding
lambda: verify data.request().ndim and buffer size match width * height *
channels (derive channels from color_space or data.shape[2]) and throw a
py::value_error if sizes mismatch; validate that k and max_iter are >= 1 (and
cast to unsigned if needed before passing to img2num_kmeans) and throw
py::value_error for invalid values; ensure any invalid color_space is rejected
similarly so img2num_kmeans is only called with well-formed inputs.
- Around line 15-39: The wrappers for gaussian_blur_fft, invert_image,
threshold_image, black_threshold_image, and bilateral_filter lack validation
that the provided py::array_t buffer size matches width*height (and optionally
channels if expected); add a buffer check in each lambda using image.request()
or image.size() and confirm it equals width * height (or width * height *
channels if the C API expects multi-channel data), also ensure the array is
contiguous and writable, and throw a descriptive pybind11::value_error or
std::invalid_argument before calling img2num_* functions (e.g., in the
gaussian_blur_fft, bilateral_filter lambdas) so callers get a clear error
instead of risking OOB memory access.
In `@example-apps/console-py/main.py`:
- Around line 15-16: The code calls cv2.imread("../../IMG_5554.jpg") and
immediately passes img into cv2.cvtColor which will raise a cryptic error if
imread returns None; modify the flow around cv2.imread and cv2.cvtColor so that
after calling cv2.imread you check if img is None (or if the file failed to
load), and if so handle it (e.g., log/raise a clear error or exit) before
calling cv2.cvtColor(img, cv2.COLOR_BGR2RGBA); keep the same variable name img
and ensure the error handling provides a helpful message about the
missing/unreadable image path.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 30-32: Add a CMake option to make building Python bindings
conditional: introduce an option like BUILD_PYTHON_BINDINGS (default ON for
non-Emscripten) and wrap the existing add_subdirectory(bindings/python) call in
an if(BUILD_PYTHON_BINDINGS) ... endif() block; also ensure the option defaults
to OFF for Emscripten builds (detect via CMAKE_SYSTEM_NAME or EMSCRIPTEN
variable) and update any README or top-level help text accordingly so users
without pybind11 can skip the Python bindings.
In `@core/CMakeLists.txt`:
- Around line 22-25: The current debug messages use message() without STATUS and
print variables (Python3_LIBRARIES, Python3_INCLUDE_DIRS) that will be empty
because find_package(Python3) requested only the Interpreter component; update
the messages to either remove the two library/include lines or change
find_package(Python3) to request the Development component if you need them, and
ensure all message calls use message(STATUS ...) for consistency with other logs
(refer to the find_package(Python3) invocation and the message(...) lines in
this diff).
In `@docker-compose.yml`:
- Line 7: Uncomment and restore the Docker Hub image line in docker-compose.yml
so the service uses ryanmillard/img2num-dev:latest (or a new versioned tag)
instead of forcing a local build; then rebuild and push an updated Docker Hub
image that includes the new Python components (tag it appropriately, e.g.
:latest or a semver tag) and update the image reference if needed so the compose
file pulls the updated image rather than building from Dockerfile.dev.
In `@Dockerfile.dev`:
- Around line 62-64: Combine the two separate RUN pip install lines into a
single RUN to avoid extra image layers and pin package versions for
reproducibility; replace the two RUN lines that call "python3 -m pip install
numpy --break-system-packages" and "python3 -m pip install opencv-python
--break-system-packages" with one RUN that installs both packages with explicit
version pins (e.g., numpy==<version> opencv-python==<version>) and include flags
like --no-cache-dir (or use a requirements.txt/constraints file) while
preserving --break-system-packages as needed.
In `@example-apps/console-py/main.py`:
- Around line 21-28: Replace the numeric literals passed into
img2num.bilateral_filter, img2num.kmeans, and img2num.labels_to_svg with named
constants (e.g., BILATERAL_DIAMETER, BILATERAL_SIGMA_COLOR,
BILATERAL_SIGMA_SPACE, KMEANS_CLUSTERS, KMEANS_ITERATIONS, KMEANS_ATTEMPTS,
SVG_SCALE) and add short inline comments where those constants are declared to
explain their meaning; update calls in the main function (the usages of
img2num.bilateral_filter, img2num.kmeans, and img2num.labels_to_svg) to use
those constant names instead of magic numbers so the parameters are
self-describing and easy to tweak for examples.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11d60ad0-e975-43a4-bccf-c33f2831930c
📒 Files selected for processing (8)
CMakeLists.txtDockerfile.devbindings/c/CMakeLists.txtbindings/python/CMakeLists.txtbindings/python/src/img2num_pybind.cppcore/CMakeLists.txtdocker-compose.ymlexample-apps/console-py/main.py
Owner
|
I'm building it right now.😓 We need a Docker god to help us here. These builds take far too long. |
Owner
|
How can I message you privately? As the days go by, I keep feeling the need to have a better way to communicate because this is becoming a bit tough. A full Discord server might be overkill for us right now, but I'm not sure. I think we need a way to keep others updated and also message privately. |
Collaborator
Author
Yes :( Dawn takes forever |
Repository owner
deleted a comment from
Krasner
Apr 11, 2026
Owner
|
Thanks for the information - I’ve deleted your message containing your email address just in case. I did email you and you can access the DockerHub repo here: https://hub.docker.com/repository/docker/ryanmillard/img2num-dev/general |
Collaborator
Author
Thanks! Got your email. Will reply a bit later |
Collaborator
Author
|
@Ryan-Millard I updated the docker image (on dockerhub). seems to be working fine. It now included the necessary additions for python binding builds |
Owner
Thanks! I realised that our Please write some documentation for this - we need our users to know how to use the library once they import it. I also noticed that you're referencing the Pybind build files directly from the ryan@Ryans-PC:~/projects/Img2Num$ ls packages/js/
build-wasm imageToUint8ClampedArray.js imageToUint8ClampedArray.test.js index.js package.json safeWasmWrappers.js tsconfig.typedoc.json wasmClient.js wasmWorker.js
ryan@Ryans-PC:~/projects/Img2Num$ |
Collaborator
Author
|
Yeah I wasn't sure how to place the generated python lib... maybe i'll place it in |
Collaborator
Author
|
OK i made it so that the python library builds to the system (docker) python path: That way in |
Owner
|
I don't think it should build as a I'll clean it up - don't worry. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/CMakeLists.txt`:
- Around line 23-33: The CMakeLists currently sets LIBRARY_OUTPUT_DIRECTORY to
the system Python site-packages (${Python3_SITEARCH}), which writes build
artifacts into the install tree; revert that by removing or resetting the
LIBRARY_OUTPUT_DIRECTORY change for the img2num target and instead add an
install(TARGETS img2num ...) stanza to perform deployment to
${Python3_SITEARCH}. Specifically, stop assigning LIBRARY_OUTPUT_DIRECTORY
"${Python3_SITEARCH}" for target img2num, ensure build outputs go to the default
build tree, and add/install the commented install(TARGETS img2num COMPONENT
python LIBRARY DESTINATION "${Python3_SITEARCH}" RUNTIME DESTINATION
"${Python3_SITEARCH}") (and re-enable include(GNUInstallDirs) if needed) so
installation is performed via CMake install with CMAKE_INSTALL_PREFIX.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed1fe177-3f28-4ef5-a4b6-7c145e8816e2
📒 Files selected for processing (2)
bindings/python/CMakeLists.txtexample-apps/console-py/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- example-apps/console-py/main.py
Owner
|
Sorry, #320 doesn't cover that, but it's the same thing. |
Owner
|
/docker-build 2b48c4c |
Collaborator
Author
|
@Ryan-Millard python ci build works now! |
Owner
|
/docker-build e9cc8c4 |
Collaborator
Author
|
Let's merge this? |
Owner
Sure. We can handle the below in another PR. @coderabbitai, plase make issues for the below (they are all Python related for PR #307): TODO:
Also, do you think we need anything else? |
This was referenced Apr 25, 2026
Open
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 161 🏁 Script executed: Length of output: 51
On your question about whether anything else is needed — yes, I'd suggest one more:
Beyond that, the PR looks solid. The pybind11 module is well-structured, ✏️ Learnings added
🧠 Learnings used |
Owner
|
Please suggest a conventional commit for this PR. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
A few notes on the choices:
🧠 Learnings used |
This was referenced Apr 25, 2026
Ryan-Millard
added a commit
that referenced
this pull request
Apr 29, 2026
…iles (#311) Add a full `.coderabbit.yaml` configuration for CodeRabbit, covering: - Assertive review profile with request-changes workflow enabled - Granular path_instructions for C/C++ core, WGSL shaders, Docusaurus docs, GitHub Actions workflows, scripts, Dockerfiles, bindings, and example-apps - Pre-merge checks for Conventional Commit titles, non-boilerplate descriptions, and a custom "CodeRabbit Config Needs Update" check that warns when new languages or tooling are introduced without updating .coderabbit.yaml - Tool enablement scoped to the repo's stack (clang, cppcheck, shellcheck, hadolint, eslint, actionlint, etc.); Python tooling deferred to `#307` - Knowledge base, code generation (Doxygen/JSDoc), and unit test guidance Refactor `.github/dependabot.yml` from a single root npm entry to scoped per-directory groups (root, packages/js, example-apps/react-js, docs), with separate production/development dependency groups and updated commit prefixes. Extend `.github/CODEOWNERS` to assign ownership of example-apps subtrees by language (example-apps/*-c, *-cpp, *-py) and the Python bindings directory (/bindings/py) to `@Ryan-Millard` and `@Krasner`. Consolidate licensing: rewrite root `LICENSE` to the full standard MIT License text, remove the now-redundant `LICENSE-MIT` file, and strip the informational MIT preamble notes from `.github/LICENSE`, `docs/LICENSE`, and `example-apps/react-js/LICENSE`. Update `README.md` to replace the bullet-based licensing summary with a table linking to per-directory AGPLv3 license files.
This was referenced Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enable python bindings as part of the c/cpp compilation. Update Dockerfile to have necessary python components.
IMPORTANT: Update docker container on dockerhub!
As part of building the c/cpp apps python bindings will be built.
Note this will generate the
img2numpython lib inbuild-c-cpp/bindings/python/What was changed & why
Fixes: #
Changes
Testing & Verification
Additional Resources
Summary by CodeRabbit
New Features
Bug Fixes
Chores