feat: integrate https://github.com/nilsnolde/osrm-bindings#7485
feat: integrate https://github.com/nilsnolde/osrm-bindings#7485DennisOSRM merged 74 commits intoProject-OSRM:masterfrom
Conversation
| { text: 'Node.js API', link: '/nodejs/api' }, | ||
| { text: 'Python', items: [ | ||
| { text: 'API', link: '/python/api' }, | ||
| { text: 'Development', link: '/python/development' } |
There was a problem hiding this comment.
I tried to be really thorough, so I had claude add some python development and API docs. python doesn't work very well with vitepress with pulling out docstrings etc. it's mostly mkdocs these days in python land. not sure, but I could imagine mkdocs support JS docstrings as well. I guess you just switched to vitepress @DennisOSRM , not sure you'd be open to ditch it again potentially?
There was a problem hiding this comment.
for reference, this is the mkdocs based documentation I configured for valhalla: https://valhalla.github.io/valhalla/
There was a problem hiding this comment.
Sure, we can discuss the feasibility of switching. Perhaps independent of this PR, but whatever we choose should be visually on par with vitepress.
| @@ -0,0 +1,68 @@ | |||
| # osrm-bindings | |||
|
|
|||
| **Python bindings for [osrm-backend](https://github.com/Project-OSRM/osrm-backend) using [nanobind](https://github.com/wjakob/nanobind).** | |||
There was a problem hiding this comment.
this file is the project page on pypi.org
| @ECHO OFF | ||
| SETLOCAL EnableDelayedExpansion | ||
|
|
||
| SET DATA_DIR=%CD% |
There was a problem hiding this comment.
could probably use the standard Makefile instead of duplicating the same logic here. I know there gmake for windows, could just work..
There was a problem hiding this comment.
Perhaps after merging this change 😉
|
The PR probably needs a rebase onto latest master to pick up a fix for macOS ci. |
|
jeez, thanks, was just about to diagnose! |
|
@DennisOSRM this is ready for review again. I'll just have to revert the last commit before merging, where I'm testing if the wheels build properly, without uploading to pypi. as for the "open questions": maybe it's best to continue releasing under my account. it's already on pypi and it's the most frictionless path IMO. pypi doesn't have the concept of "org" namespaces as e.g. npm does (otherwise we wouldn't have the mess with |
|
I also wanna say: in effect osrm is not buildable anymore with just system packages in many cases. since it removed all vendored packages and e.g. brew removed even if in theory vcpkg might be optional in cmake, in practice it's not on the majority of platforms. IMHO it was nicer before. what wasn't nice is that is was hard-vendored instead of submodule'd (in terms of maintenance). there's a way to have submodules (at least for header-only deps) to fall back to. I'm really more OSS than FOSS, but in such cases I'm all for "free". personally I really don't like being kinda locked in to smth like vcpkg (still, it's MS) without a hassle. that's just my 2c from the packaging experience of the python bindings. |
…to nn-py-bindings
|
was mostly battling x64 macos in the past commits. now everything's working and all wheels are uploaded as artifacts: https://github.com/Project-OSRM/osrm-backend/actions/runs/25164784497. next I wanna try linux aarch64, but after merging:) |
| CI wheel builds run inside a custom manylinux image | ||
| ([nilsnolde/manylinux](https://github.com/nilsnolde/manylinux), branch | ||
| `osrm_python`) that ships vcpkg pre-bootstrapped at the SHA pinned in | ||
| `vcpkg-configuration.json`, plus a pre-warmed vcpkg binary cache compiled |
There was a problem hiding this comment.
What happens when the vcpkg pin changes? Will that incur a slow build of the many linux image once?
There was a problem hiding this comment.
mostly right. the barrier is basically whether any vcpkg update in upstream osrm-backend (only new version for whatever reason, new package etc) would break the OSRM compilation or python tests with the previous vcpkg still present in the previous manylinux image. if it does, the PR workflow will fail, I'll need to workflow_dispatch with the osrm-backend commit sha introducing that vcpkg change (i.e. build a new tagged image). if it doesn't, we don't have to care much about them being not in sync currently.
it's a bummer but kinda fair. the python builds failing won't be too obnoxious. they're the last steps to run, and only on a selection of jobs.
I do feel responsible for this now, but I invited at least you @DennisOSRM as collaborator in case I'm not available. eventually we could move the repo to the Project-OSRM org?
| @ECHO OFF | ||
| SETLOCAL EnableDelayedExpansion | ||
|
|
||
| SET DATA_DIR=%CD% |
There was a problem hiding this comment.
Perhaps after merging this change 😉
|
thanks @DennisOSRM for the quick review. there was a master conflict, also changed a tiny thing in 4ed0a95. |
|
urgh, are you open to a force merge (if you have to) after I resolve this conflict and merge master @DennisOSRM ? lots of conflicts 😅 |
Head branch was pushed to by a user without write access
|
Thanks so much for the contribution. Glad, we could get it in this weekend. |
forgot to add here: @whytro did the real work during GSoC 2023. this PR is "just" integrating it into upstream.
getting to build linux was less of a hassle than I imagined. it's more of a hassle to review I guess.. my proposal: since it's mostly copy/paste from https://github.com/nilsnolde/osrm-bindings, it's not too bad to not review the code toooo much. one can somewhat trust the python packaging pipeline. I'll add more details later. IMO it's more important to understand how things work conceptually, so I'll add quite some more text to the PR description once this is a final PR. and of course feel free to ask any understanding question you have:) packaging is always awful, but honestly, python got really usable after they introduced (and widely supported) pyproject.toml.
General
nanobindfor C++ binding (i.e. no setup.py, all CMake based), very close to libosrm API, only some plumbing python codeRepo Layout
everything is properly namespaced to python:
src/python/ ├── CMakeLists.txt ├── README.md ├── include/ │ └── python/ │ ├── engineconfig_nb.hpp │ ├── parameters/ │ ├── types/ │ └── utility/ ├── osrm/ │ ├── __init__.py │ ├── __main__.py │ └── osrm_ext.pyi └── src/ ├── engineconfig_nb.cpp ├── osrm_nb.cpp ├── parameters/ ├── types/ └── utility/ test/python/ ├── constants.py ├── test_index.py ├── test_match.py ├── test_nearest.py ├── test_route.py ├── test_table.py ├── test_tile.py └── test_trip.pyPackaging Architecture
cibuildwheelto orchestrate the build, packaging, repairing (mostly vendor dynamic libs into the wheel), can be run locally toomanylinux_2_28docker image based on almalinux 8. I forked https://github.com/pypa/manylinux a while ago to https://github.com/nilsnolde/manylinux, where I added the OSRM build dependencies for both aarch64 & x86: https://github.com/nilsnolde/manylinux/pkgs/container/manylinux.release-monthly.ymlOpen Questions
osrm-bindingsis ok-ish,osrmwould of course be much nicer:)