Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial “mojo4py” proof-of-concept that exposes Decimo’s BigDecimal to Python via a Mojo-built CPython extension module (_decimo.so), with a thin Python wrapper (decimo.Decimal) providing Pythonic dunder/operator behavior. The PR also reorganizes shell test runners, integrates Python formatting (ruff) into tooling, and expands CI into parallel jobs.
Changes:
- Introduces
python/decimo_module.mojo(_decimoextension),python/decimo.py(Python wrapper), and Python-side tests/stubs. - Refactors
tests/test_all.shinto smaller test runners and adds a top-leveltests/test_decimo.shaggregator. - Updates
pixi.toml, pre-commit, and GitHub Actions workflow to incorporate the new Python flow and formatting.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tomlmojo.sh | Simplifies the TOMLMojo test runner. |
| tests/test_decimo.sh | New aggregator script to run decimo-related Mojo test suites. |
| tests/test_decimal128.sh | New Decimal128 test runner script. |
| tests/test_binary.sh | Removed legacy script in favor of per-suite runners. |
| tests/test_biguint.sh | New BigUInt test runner script. |
| tests/test_bigint10.sh | New BigInt10 test runner script. |
| tests/test_bigint.sh | New BigInt test runner script. |
| tests/test_bigdecimal.sh | New BigDecimal test runner script. |
| tests/test_big.sh | Removed legacy script in favor of per-suite runners. |
| tests/test_all.sh | Now delegates to the new suite-level runners. |
| tests/cli/test_evaluator.mojo | Updates docstrings in evaluator tests. |
| python/tests/test_decimo.py | Adds Python “Phase 0” interop tests for the wrapper/extension. |
| python/decimo_module.mojo | Adds Mojo bindings to build the _decimo CPython extension module. |
| python/decimo.py | Adds the Python Decimal wrapper delegating to the _decimo extension. |
| python/_decimo.pyi | Adds type stubs for the _decimo extension API. |
| pixi.toml | Adds Python build/test tasks and ruff formatting; reorganizes tasks. |
| pixi.lock | Locks the added ruff dependency. |
| docs/plans/mojo4py.md | Adds a detailed design/roadmap document for Mojo→Python bindings. |
| LICENSE | Replaces the Apache template placeholder copyright line. |
| .pre-commit-config.yaml | Splits formatting hooks: Mojo format for .mojo, ruff for .py. |
| .gitignore | Ignores Python build artifacts (*.so, __pycache__, *.pyc). |
| .github/workflows/run_tests.yaml | Reworks CI into parallel jobs and adds Python/format checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Build CLI binary | ||
| run: pixi run buildcli | ||
| - name: Run tests | ||
| run: bash ./tests/test_cli.sh |
There was a problem hiding this comment.
The workflow runs pixi run build, but pixi.toml no longer defines a build task (it defines buildcli). This step will fail at runtime unless build is reintroduced or the workflow is updated to call the new task name.
| - name: Build & run Python tests | ||
| run: pixi run testpy | ||
|
|
There was a problem hiding this comment.
The workflow runs pixi run pytest, but pixi.toml does not define a pytest task (it defines testpy). This job will fail unless the task name is aligned (either add a pytest task/alias or update the workflow to run the existing Python test task).
python/decimo.py
Outdated
| return not self._inner.lt(other._inner) | ||
|
|
||
| def __ne__(self, other): | ||
| return not self.__eq__(other) |
There was a problem hiding this comment.
__ne__ is implemented as not self.__eq__(other). If __eq__ returns NotImplemented (e.g., comparing to an unconvertible type), this will incorrectly return False instead of propagating NotImplemented. Handle the NotImplemented case explicitly or implement __ne__ via the underlying _inner comparison.
| return not self.__eq__(other) | |
| eq_result = self.__eq__(other) | |
| if eq_result is NotImplemented: | |
| return NotImplemented | |
| return not eq_result |
| def __bool__(self): | ||
| return str(self) != "0" | ||
|
|
There was a problem hiding this comment.
__bool__ currently checks str(self) != "0", but decimo can stringify negative zero as "-0" (noted in the Python tests), which would incorrectly evaluate as truthy. Consider implementing truthiness using a numeric comparison to zero (e.g., via _inner.eq(...)) or normalizing the string check to treat both "0" and "-0" as false.
python/decimo.py
Outdated
| other = Decimal(other) | ||
| return self._inner.lt(other._inner) | ||
|
|
||
| def __le__(self, other): | ||
| if not isinstance(other, Decimal): | ||
| other = Decimal(other) | ||
| return self._inner.le(other._inner) | ||
|
|
||
| def __gt__(self, other): | ||
| if not isinstance(other, Decimal): | ||
| other = Decimal(other) | ||
| return not self._inner.le(other._inner) | ||
|
|
||
| def __ge__(self, other): | ||
| if not isinstance(other, Decimal): | ||
| other = Decimal(other) |
There was a problem hiding this comment.
__lt__/__le__/__gt__/__ge__ eagerly coerce other to Decimal and will raise if conversion fails, unlike __eq__ which returns NotImplemented on unconvertible inputs. For Python operator semantics (and to allow the reflected comparison to run), these methods should also return NotImplemented when other cannot be converted.
| other = Decimal(other) | |
| return self._inner.lt(other._inner) | |
| def __le__(self, other): | |
| if not isinstance(other, Decimal): | |
| other = Decimal(other) | |
| return self._inner.le(other._inner) | |
| def __gt__(self, other): | |
| if not isinstance(other, Decimal): | |
| other = Decimal(other) | |
| return not self._inner.le(other._inner) | |
| def __ge__(self, other): | |
| if not isinstance(other, Decimal): | |
| other = Decimal(other) | |
| try: | |
| other = Decimal(other) | |
| except Exception: | |
| return NotImplemented | |
| return self._inner.lt(other._inner) | |
| def __le__(self, other): | |
| if not isinstance(other, Decimal): | |
| try: | |
| other = Decimal(other) | |
| except Exception: | |
| return NotImplemented | |
| return self._inner.le(other._inner) | |
| def __gt__(self, other): | |
| if not isinstance(other, Decimal): | |
| try: | |
| other = Decimal(other) | |
| except Exception: | |
| return NotImplemented | |
| return not self._inner.le(other._inner) | |
| def __ge__(self, other): | |
| if not isinstance(other, Decimal): | |
| try: | |
| other = Decimal(other) | |
| except Exception: | |
| return NotImplemented |
python/decimo_module.mojo
Outdated
| from python import PythonObject, Python | ||
| from python.bindings import PythonModuleBuilder | ||
| from os import abort | ||
| from memory import UnsafePointer |
There was a problem hiding this comment.
Python and UnsafePointer are imported but not used in this module. Removing unused imports will keep the binding module minimal and avoid potential lint/compile warnings as the Mojo toolchain evolves.
| from python import PythonObject, Python | |
| from python.bindings import PythonModuleBuilder | |
| from os import abort | |
| from memory import UnsafePointer | |
| from python import PythonObject | |
| from python.bindings import PythonModuleBuilder | |
| from os import abort |
pixi.toml
Outdated
| test_cli = "bash tests/test_cli.sh" | ||
| bcli = "clear && pixi run buildcli" | ||
| buildcli = "pixi run mojo build -I src -I src/cli -o decimo src/cli/main.mojo" | ||
| tcli = "clear && pixi run clitest" |
There was a problem hiding this comment.
bcli and tcli call clibuild/clitest, but only buildcli and testcli are defined below. As written, pixi run bcli / pixi run tcli will fail with an unknown task; either rename buildcli→clibuild and testcli→clitest, or update bcli/tcli to reference the existing task names.
| tcli = "clear && pixi run clitest" | |
| tcli = "clear && pixi run testcli" |
| bpy = "clear && pixi run buildpy" | ||
| buildpy = """pixi run mojo build python/decimo_module.mojo \ | ||
| --emit shared-lib -I src -o python/_decimo.so""" | ||
| testpy = "pixi run buildpy && pixi run python python/tests/test_decimo.py" | ||
| tpy = "clear && pixi run testpy" |
There was a problem hiding this comment.
testpy runs pixi run pybuild, but there is no pybuild task—this file defines buildpy (and bpy as an alias). This will break pixi run testpy; update testpy to call the correct build task name or add a pybuild alias for consistency with the docs.
_decimo.so
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| out self: BigDecimal, args: PythonObject, kwargs: PythonObject | ||
| ) raises: | ||
| """Construct a BigDecimal from a single argument (string, int, or float). |
There was a problem hiding this comment.
bigdecimal_py_init accepts kwargs but never checks/uses it. Calling _decimo.BigDecimal(value="1.23") will produce a misleading positional-args error rather than a clear "no keyword arguments" / keyword support. Either explicitly reject non-empty kwargs with a Python-like error, or implement keyword handling (e.g., value).
| "Decimal() takes exactly 1 argument (" | ||
| + String(len(args)) | ||
| + " given)" | ||
| ) | ||
| # Convert any Python object to its string representation, then construct. |
There was a problem hiding this comment.
The raised constructor error says Decimal() even though this is the _decimo.BigDecimal type. This will be confusing for users calling the extension type directly and for debugging. Adjust the message to reference BigDecimal() (or _decimo.BigDecimal()) to match the actual callable.
|
|
||
| Usage from Python: | ||
| Decimal("3.14") | ||
| Decimal(42) | ||
| Decimal(3.14) # via str() conversion | ||
| """ | ||
| if len(args) != 1: |
There was a problem hiding this comment.
This docstring claims the constructor accepts int/float, but the Python wrapper always passes str(value) and the binding currently just does String(args[0]). If non-string Python objects aren’t actually supported here, update the docstring to avoid promising behavior that callers of _decimo.BigDecimal won’t get.
| identification within third-party archives. | ||
|
|
||
| Copyright [yyyy] [name of copyright owner] | ||
| Copyright 2026 Yuhao Zhu |
There was a problem hiding this comment.
This is part of the Apache-2.0 license appendix template (“How to apply…”). Editing the template text (e.g., replacing the bracketed copyright line) makes the LICENSE file diverge from the standard license text and can create ambiguity for downstream tooling. Keep the LICENSE text verbatim and put the project’s actual copyright notice in a NOTICE file and/or in source file headers.
| Copyright 2026 Yuhao Zhu | |
| Copyright [yyyy] [name of copyright owner] |
| ```toml | ||
| # python bindings (mojo4py) | ||
| pybuild = "pixi run mojo build python/decimo_module.mojo --emit shared-lib -I src -o python/_decimo.so" | ||
| pytest = "pixi run pybuild && pixi run python python/tests/test_decimo.py" | ||
| ``` | ||
|
|
||
| - `pixi run pybuild` — compiles the Mojo binding to `python/_decimo.so`. | ||
| - `pixi run pytest` — builds then runs the Python test suite. |
There was a problem hiding this comment.
This plan doc references pixi run pybuild / pixi run pytest, but the actual tasks in pixi.toml are buildpy / testpy (and aliases bpy / tpy). Update the documentation snippet and quick-start commands to match the current task names so readers can follow it without errors.
| pixi run pybuild # Compiles python/decimo_module.mojo → python/_decimo.so | ||
| pixi run pytest # Builds, then runs python/tests/test_decimo.py |
There was a problem hiding this comment.
Quick-start uses pixi run pybuild / pixi run pytest, but those tasks don’t exist in the current pixi.toml (it defines buildpy / testpy). Adjust these commands to the real task names to keep the quick-start copy/pasteable.
| pixi run pybuild # Compiles python/decimo_module.mojo → python/_decimo.so | |
| pixi run pytest # Builds, then runs python/tests/test_decimo.py | |
| pixi run buildpy # Compiles python/decimo_module.mojo → python/_decimo.so | |
| pixi run testpy # Builds, then runs python/tests/test_decimo.py |
| from python import PythonObject | ||
| from python.bindings import PythonModuleBuilder | ||
| from os import abort | ||
|
|
||
| from decimo import BigDecimal |
There was a problem hiding this comment.
Unused imports: Python and UnsafePointer are imported but not referenced in this module. Removing them avoids lint noise and keeps the binding module minimal.
…thon via `_decimo.so` (#179) Adds an initial “use mojo in python” proof-of-concept that exposes Decimo’s `BigDecimal` to Python via a Mojo-built CPython extension module (`_decimo.so`), with a thin Python wrapper (`decimo.Decimal`) providing Pythonic dunder/operator behavior. The PR also reorganizes shell test runners, integrates Python formatting (ruff) into tooling, and expands CI into parallel jobs. **Changes:** - Introduces `python/decimo_module.mojo` (`_decimo` extension), `python/decimo.py` (Python wrapper), and Python-side tests/stubs. - Refactors `tests/test_all.sh` into smaller test runners and adds a top-level `tests/test_decimo.sh` aggregator. - Updates `pixi.toml`, pre-commit, and GitHub Actions workflow to incorporate the new Python flow and formatting.
Adds an initial “use mojo in python” proof-of-concept that exposes Decimo’s
BigDecimalto Python via a Mojo-built CPython extension module (_decimo.so), with a thin Python wrapper (decimo.Decimal) providing Pythonic dunder/operator behavior. The PR also reorganizes shell test runners, integrates Python formatting (ruff) into tooling, and expands CI into parallel jobs.Changes:
python/decimo_module.mojo(_decimoextension),python/decimo.py(Python wrapper), and Python-side tests/stubs.tests/test_all.shinto smaller test runners and adds a top-leveltests/test_decimo.shaggregator.pixi.toml, pre-commit, and GitHub Actions workflow to incorporate the new Python flow and formatting.