Test Python + NumPy in CI#405
Conversation
564a865 to
39c39a6
Compare
| on: [push, pull_request] | ||
|
|
||
| jobs: | ||
| check_format: |
There was a problem hiding this comment.
The format job wasn't working correctly anymore – the default Python version in Azure Pipelines no longer works with our version of black – so it needed maintenance. I moved it to GitHub Actions so all the Python CI would be in one place.
If you'd like to remove Azure Pipelines entirely, I could move the one remaining job to GitHub Actions in a follow-on PR.
There was a problem hiding this comment.
yes please remove Azure Pipelines. Centralize CI on Github Actions.
There was a problem hiding this comment.
Will open a follow-on for that.
|
@syoyo Would you like to review these open PRs before I merge them? |
|
@paulmelnikow reviewed! |
There was a problem hiding this comment.
Pull request overview
Adds a Python-focused CI pipeline and a small Python test project to validate the published/built tinyobjloader wheels across multiple Python + NumPy versions, targeting regressions described in issues #400–#402.
Changes:
- Introduces a
tests/python/mini project (uv + pytest) with functional tests coveringnumpy_*binding behaviors. - Replaces the previous GitHub Actions wheel workflow with a new consolidated workflow that builds wheels and runs the Python/NumPy matrix tests.
- Updates formatting configuration and minor whitespace/CI cleanup (Black excludes, Azure pipeline trimming, etc.).
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/python.yml |
New workflow: format check, quick manylinux build, wheel test matrix for Python/NumPy, full wheel builds, sdist, and publish. |
.github/workflows/wheels.yml |
Removes the prior “build and upload to PyPI” workflow in favor of python.yml. |
tests/python/tinyobjloader_tests/test_loader.py |
Adds NumPy-focused regression tests for issues #400/#401/#402. |
tests/python/tinyobjloader_tests/loader.py |
Adds a small ObjReader wrapper to simplify tests. |
tests/python/tinyobjloader_tests/__init__.py |
Initializes the test package. |
tests/python/pyproject.toml |
Defines the Python test project (deps + build backend). |
tests/python/uv.lock |
Locks the Python test project dependencies for uv. |
tests/python/README.md |
Brief README for the Python test project. |
pyproject.toml |
Extends Black configuration with force-exclude patterns. |
setup.py |
Formatting-only tweaks and removes unused long_description loading. |
azure-pipelines.yml |
Removes disabled Python-wheel-related pipeline configuration, leaving unit tests. |
fuzzer/runner.py |
Minor whitespace cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| readme = "README.md" | ||
| requires-python = ">=3.9" | ||
|
|
||
| dependencies = ["pytest>=8.0", "black==22.10.0"] |
There was a problem hiding this comment.
The test project imports NumPy (e.g. import numpy as np in test_loader.py), but numpy is not listed as a dependency here. CI installs NumPy manually, but local runs like uv sync && uv run pytest will fail with ModuleNotFoundError. Consider adding numpy as a dependency (with a broad lower bound) and then overriding the version in CI as you already do.
| dependencies = ["pytest>=8.0", "black==22.10.0"] | |
| dependencies = ["pytest>=8.0", "black==22.10.0", "numpy>=1.20"] |
There was a problem hiding this comment.
I think this might not be a good idea. This would result in a version that's pinned in the lockfile, and then in CI, it's going to install that version. If that version doesn't have a wheel for the later versions of Python, it's going to result in a slow running build.
It's not great that the local environment requires a manual install. Buy maybe documenting that would be enough.
| def test_numpy_face_vertices_two_quads(): | ||
| """ | ||
| Test for https://github.com/tinyobjloader/tinyobjloader/issues/400 | ||
| """ | ||
|
|
There was a problem hiding this comment.
These tests assert the fixed behavior for open issues (per PR description, several currently fail). As written, they will fail the test_wheels CI job and block merges. If the underlying bug fix isn’t part of this PR, consider marking the known-failing ones as xfail/skipped with an issue link (or splitting into a follow-up PR that lands together with the fix) so CI stays green.
There was a problem hiding this comment.
Seems not necessary, since we're about to merge a PR with a fix.
|
@paulmelnikow also ran Copilot review. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Based on discussion at #405 (comment)
Add test tasks to GitHub Actions which test the generated wheels using different Python and NumPy versions. It seems to be working well at this point. I've added four tests for issues #400 and they are all failing in the way I expect – surfacing disparities between
num_face_verticesandnumpy_num_face_vertices().I've also added tests for issue #401 which is passing, and #402 is also covered in the #400 tests so those will need further investigation at a later time.
Closes #403