Add experimental Windows wheel build support#117
Open
zym1998year wants to merge 3 commits into
Open
Conversation
The Windows SDK ``ctype.h`` defines preprocessor macros named ``_C1``, ``_C2``, ``_C3`` (control-character classification masks). Including ``Windows.h`` -- which any Windows MSVC translation unit eventually does -- expands those macros over the SellmeierMedium private members of the same name and the file no longer compiles. Rename the private members to ``_sellC1`` / ``_sellC2`` / ``_sellC3`` and update the header, the constructor's member-initializer list, the getN() formula, and the GPU device-pointer constructor accordingly. Public API and Sellmeier coefficient ordering are unchanged.
Three independent MSVC fixes in src/: - src/batoid.cpp: MSVC's OpenMP requires a signed integral loop counter; convert the two ``#pragma omp parallel for`` loops in applyForwardTransformArrays / applyReverseTransformArrays from ``size_t i`` to ``ptrdiff_t i`` (with a ``ptrdiff_t nloop`` mirror of ``size_t n`` to avoid clipping). Adds ``<cstddef>`` for the type. Other parallel loops in this file already used ``int`` and did not need touching. - src/obscuration.cpp: replace the C++ alternative tokens ``or`` in the polygon point-in-polygon and grid bounds checks with ``||``; MSVC parses ``or`` as an identifier without ``/Za`` + ``ciso646``. - src/table.cpp: same alternative-token replacement on the two bounds-check expressions in ``Table::eval`` and the gradient variant. No behavioural change on Linux/macOS; the ``size_t``/``ptrdiff_t`` swap is safe for the practical ray-vector sizes batoid handles.
Two adjustments to ``setup.py``'s ``CMakeBuild.build_extension``: - Drop the unconditional ``-- -j8`` forwarded to the underlying build tool. MSBuild rejects ``-j8`` outright (it expects ``/m`` / ``-maxCpuCount``); Make/Ninja accept it but only for historical reasons. Hand parallelism to ``cmake --build --parallel`` instead, honouring ``CMAKE_BUILD_PARALLEL_LEVEL`` when set, so the same code path works for every CMake generator. - Add ``-Dpybind11_DIR=`` from ``pybind11.get_cmake_dir()`` to the CMake configure args. PEP 517 isolated builds install pybind11 in a temporary directory that CMake's ``find_package(pybind11)`` cannot otherwise discover, so the configure step would fail; this hint makes ``pip install . -v`` and ``pip wheel .`` work end-to-end on Windows without manually exporting CMAKE_PREFIX_PATH. The Linux/macOS path is unaffected: those generators already accept ``--parallel`` and pybind11 is normally on CMake's default search path there.
44c3a87 to
c606b79
Compare
Owner
|
Hi! Thanks for this. Same basic comments as the GalSim PR:
I'll take a closer look when that's ready. |
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
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.
Draft PR against
releases/0.8(baseline367a861, v0.8.0).Branch on file:
windows-port.Status
pip install . -v(Ninja+MSVC),pip wheel,delvewheel repair, fresh-venv install with separately-built GalSimWindows wheel (no conda PATH),
import batoid, LSST yaml load,raytrace, SellmeierMedium, ObscPolygon, Bicubic, and 176 / 204
pytest cases passing.
tests/test_notebooks.py, allModuleNotFoundError: No module named 'nbformat'— jupyter testinfra not installed. Not Windows-specific.
member rename and the
or->||swap (which GCC/Clang acceptidentically). Existing CI should pass without changes; not re-run
from this branch.
Dependency
batoid depends on GalSim. GalSim does not currently ship Windows
wheels; a companion PR adds Windows support to GalSim
(
releases/2.8branch,windows-msvc-port). Once that PR or asubsequent GalSim release exposes a
win_amd64wheel on PyPI, thisPR's wheel becomes installable purely with
pip install. Until then,end users on Windows would need to install GalSim from the companion
branch.
This PR does not change
install_requires=['galsim', ...]; it onlyenables batoid itself to build.
Summary
Three small commits make batoid build, install, and pass its
non-notebook test suite on native Windows x64 with the MSVC
toolchain. The patches address one Windows-SDK macro collision, three
independent MSVC source-level issues, and two CMake / pybind11 build
quirks. Diff totals +40 / -11 across six files.
What this PR changes
include/medium.h,src/medium.cppsrc/batoid.cpp,src/obscuration.cpp,src/table.cppsetup.pyBranch tip:
d28bc8a.Per-commit detail (click to expand)
1. SellmeierMedium macro collision
The Windows SDK header
ctype.hdefines preprocessor macros named_C1,_C2,_C3(control-character classification masks).Including
Windows.h— which any Windows MSVC translation uniteventually does — expands those macros over the matching
SellmeierMedium private members and the file no longer parses.
Rename to
_sellC1/_sellC2/_sellC3with a one-line commentin the header explaining why. Updates the constructor's
member-initializer list,
getN(), and theBATOID_GPUdevice-pointerconstructor. Public API and Sellmeier coefficient ordering unchanged.
2. C++ sources for MSVC
Three independent fixes:
src/batoid.cpp: convert the two#pragma omp parallel forloopsin
applyForwardTransformArrays/applyReverseTransformArraysfrom
size_t itoptrdiff_t i(with aptrdiff_t nloop = static_cast<ptrdiff_t>(n)mirror so the upper bound width ispreserved). MSVC OpenMP requires a signed integral loop counter.
Other parallel loops in this file already used
int iand did notneed touching. Adds
<cstddef>forptrdiff_t.src/obscuration.cpp: replaceorwith||in the polygonpoint-in-polygon test and the y-grid bounds early-exit. MSVC parses
oras an identifier without/Zaplus<ciso646>.src/table.cpp: replaceorwith||in the two bounds-checkexpressions in
Table::evaland the gradient variant.3. CMake extension build for Windows
setup.pyonly:build_args += ["--", "-j8"]and replace withbuild_args += ["--parallel", str(jobs)]driven byCMAKE_BUILD_PARALLEL_LEVEL, falling back to a bare--parallel.MSBuild rejects
-j8outright (it expects/m/-maxCpuCount);--parallelis generator-agnostic.-Dpybind11_DIR=frompybind11.get_cmake_dir()tocmake_args, guarded by a try/except aroundimport pybind11.PEP 517 isolated builds install pybind11 in a temporary directory
that CMake's
find_package(pybind11)cannot otherwise discover.Verification
Built and tested on Windows 11 x64, Python 3.11.15 (conda-forge), VS
2026 Community (MSVC 14.50.35717), CMake 3.30, Ninja 1.13, pybind11
3.0.3.
Build / wheel
msvcp140.dllvcruntime140.dll,vcruntime140_1.dllvcomp140.dllpython311.dll,kernel32.dll,api-ms-win-crt-*batoid does not link FFTW directly (the FFT dependency goes through
GalSim), so the batoid wheel does not bundle
fftw3.dll.Smoke (touches each patch)
import batoid0.1.dev1069+gd28bc8a36SellmeierMedium(BK7).getN(550 nm)1.5185223876207927Optic.fromYaml('LSST_r.yaml')ObscPolygon([0,1,1,0],[0,0,1,1]).contains(0.5,0.5) / (2,2)True/FalseorBicubicof0.01*(x*x+y*y), sag at (0.2, 0.3) and off-grid (5,5)0.0013.../nanorFull pytest
176 passed, 28 failed in 62 s. The 28 failures are all in
tests/test_notebooks.py, allModuleNotFoundError: No module named 'nbformat'(jupyter test infra not installed; not Windows-specific).The 176 passing tests cover Asphere, Bicubic, Coating, CoordSys,
CoordTransform, Lattice, Medium, OPDScreen, Obscuration, Optic,
Paraboloid, Plane, Quadric, RayVector, Sphere, Sum, Tilted, Zernike,
analysis, plot, zemax. The OpenMP parallel paths and both alt-token
sites are exercised.
Slowest test cases (top 3)
tests/test_zemax.py::test_HSC_huygensPSFtests/test_analysis.py::test_doubleZerniketests/test_Asphere.py::test_sagFresh-venv install (no conda PATH)
The cleaned PATH does not include
%CONDA_PREFIX%\Library\bin. Thesix smoke tests above pass with byte-identical numeric output to the
conda-env run, demonstrating the wheel is self-contained modulo the
GalSim wheel's bundled FFTW.
Wheel notes
msvcp140.dll,vcomp140.dll. No FFTW (usesGalSim's).
vcomp140.dllis the Microsoft OpenMP runtime, used by the loopsin
batoid.cpp/obscuration.cpp. Choosing/openmp:llvmwouldswitch this to
libomp.dll. This PR keeps the simpler/openmp(set via CMake's
find_package(OpenMP)).LSST/Rubin/HSC/DECam YAML data files, not C++ code.
Follow-ups
None blocking; each is a separate PR candidate.
windows-latestjob +cibuildwheelmatrix (
cp39 .. cp313 -win_amd64). Better aligned after theGalSim Windows wheel is upstream so batoid CI can
pip install galsimdirectly.nbformat,nbclient,ipyvolumeinCI Windows so the 28
test_notebooks.pycases run.Python_EXECUTABLE->Python3_EXECUTABLEmigration:setup.pystill passes the older form. Works today; can bemodernized in a separate PR.
Risk and compatibility
guarded by
_WIN32/ generator-aware (setup.py) or a textualswap that GCC/Clang accept identically (
or->||,size_t->ptrdiff_t, member rename).size_t itoptrdiff_t ion a 64-bit platform preserves rangefor any practical batoid input.
documented API.
delvewheelwarns when thebundled
msvcp140.dllis older than the toolset that built the.pyd. Locally I built with VS 2026 (14.50) and bundledconda-forge's 14.44 redistributable. Smoke and pytest both ran
cleanly. CI on
windows-latest(VS 2022, 14.4x) would not seethe mismatch.
Reviewer questions
it acceptable to land this batoid PR ahead of the GalSim PR (with
a note that
pip installis gated until GalSim ships a Windowswheel)?
/openmpvs/openmp:llvmfor the eventual CI/wheel build.PYTHON_EXECUTABLE->Python3_EXECUTABLEmigration: foldinto this PR, or separate?
windows-latest+ cibuildwheel workflow can followonce questions 1-2 are settled. Draft strategy available locally
if helpful.
Happy to split, rebase, or extend as the project prefers.