nilsnolde / py-osrm

Python bindings to the OSRM routing framework
https://gis-ops.github.io/py-osrm/
BSD 2-Clause "Simplified" License
8 stars 8 forks source link

Build fails in a docker container - requirements and implicit conversion warning performance #29

Open Zebreu opened 1 year ago

Zebreu commented 1 year ago

Hi, I tried to install it in a clean conda environment within a debian docker container, and I got a lot of errors:

Just the last few lines of the logs:

 /tmp/pip-build-env-a8ou_55m/overlay/lib/python3.9/site-packages/nanobind/include/nanobind/nb_call.h:65:15: error: expected ‘(’ before ‘constexpr’
       } else if constexpr (std::is_same_v<D, kwargs_proxy>) {
                 ^~~~~~~~~
  /whatsup/py-osrm/src/osrm_nb.cpp:226:1: error: expected ‘}’ at end of input
   }
   ^
  /whatsup/py-osrm/src/osrm_nb.cpp: At global scope:
  /whatsup/py-osrm/src/osrm_nb.cpp:226:1: error: expected ‘}’ at end of input
  /whatsup/py-osrm/src/osrm_nb.cpp:226:1: error: expected ‘}’ at end of input
  ninja: build stopped: subcommand failed.

  *** CMake build failed
  [end of output]

note: This error originates from a subprocess, and is likely not a problem with pip. ERROR: Failed building wheel for osrm-bindings

Are there other dependencies not listed on the readme?

whytro commented 1 year ago

Hello, I was unable to replicate this issue. Based on the logs, it seems like the compiler version might not match up to nanobind's requirements, and might be worth a check (nanobind requires a C++17 compiler: Clang 7+, GCC 8+ or MSVC 2019+).

Zebreu commented 1 year ago

I built the latest version of osrm on a ubuntu:22.04 image (since that's their recommendation), made sure I had gcc 8+, with Python 8. Is there something obvious here that's going wrong? Should I use the latest master of osrm or is the latest (5.27.1) release adequate?

root@1fe8d1aa8d21:/py-osrm# pip install . Processing /py-osrm Installing build dependencies ... done Getting requirements to build wheel ... done Installing backend dependencies ... done Preparing metadata (pyproject.toml) ... done Building wheels for collected packages: osrm-bindings Building wheel for osrm-bindings (pyproject.toml) ... error error: subprocess-exited-with-error

× Building wheel for osrm-bindings (pyproject.toml) did not run successfully. │ exit code: 1 ╰─> [41 lines of output] scikit-build-core 0.5.1 using CMake 3.22.1 (wheel) Configuring CMake... loading initial cache file build/cp38-cp38-manylinux_2_35_x86_64/CMakeInit.txt -- The CXX compiler identification is GNU 11.4.0 -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /usr/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Found Python: /opt/conda/bin/python (found suitable version "3.8.13", minimum required is "3.8") found components: Interpreter Development.Module Development.SABIModule -- Found PkgConfig: /usr/bin/pkg-config (found version "0.29.2") -- Found LibOSRM: /usr/local/lib -- Configuring done -- Generating done -- Build files have been written to: /py-osrm/build/cp38-cp38-manylinux_2_35_x86_64 *** Building project with Ninja... [1/28] Building CXX object CMakeFiles/osrm_ext.dir/src/utility/param_utility.cpp.o [2/28] Building CXX object CMakeFiles/osrm_ext.dir/src/engineconfig_nb.cpp.o FAILED: CMakeFiles/osrm_ext.dir/src/engineconfig_nb.cpp.o /usr/bin/c++ -Dosrm_ext_EXPORTS -I/py-osrm/include -I/py-osrm/include/utility -I/py-osrm/include/parameters -I/opt/conda/include/python3.8 -I/tmp/pip-build-env-y4qbag3j/overlay/lib/python3.8/site-packages/nanobind/include -std=c++17 -DBOOST_SPIRIT_USE_PHOENIX_V3 -DBOOST_RESULT_OF_USE_DECLTYPE -DBOOST_FILESYSTEM_NO_DEPRECATED -I/usr/include/lua5.2 -I/usr/local/include -I/usr/local/include/osrm -O3 -DNDEBUG -fPIC -fvisibility=hidden -fno-stack-protector -Os -ffunction-sections -fdata-sections -std=c++17 -MD -MT CMakeFiles/osrm_ext.dir/src/engineconfig_nb.cpp.o -MF CMakeFiles/osrm_ext.dir/src/engineconfig_nb.cpp.o.d -o CMakeFiles/osrm_ext.dir/src/engineconfig_nb.cpp.o -c /py-osrm/src/engineconfig_nb.cpp /py-osrm/src/engineconfig_nb.cpp: In function ‘void initEngineConfig(nanobind::module&)’: /py-osrm/src/engineconfig_nb.cpp:36:50: error: ‘default_radius’ is not a member of ‘osrm::engine::EngineConfig’ 36 | .def_rw("default_radius", &EngineConfig::default_radius) | ^~~~~~ [3/28] Building CXX object CMakeFiles/osrm_ext.dir/src/utility/osrm_utility.cpp.o FAILED: CMakeFiles/osrm_ext.dir/src/utility/osrm_utility.cpp.o /usr/bin/c++ -Dosrm_ext_EXPORTS -I/py-osrm/include -I/py-osrm/include/utility -I/py-osrm/include/parameters -I/opt/conda/include/python3.8 -I/tmp/pip-build-env-y4qbag3j/overlay/lib/python3.8/site-packages/nanobind/include -std=c++17 -DBOOST_SPIRIT_USE_PHOENIX_V3 -DBOOST_RESULT_OF_USE_DECLTYPE -DBOOST_FILESYSTEM_NO_DEPRECATED -I/usr/include/lua5.2 -I/usr/local/include -I/usr/local/include/osrm -O3 -DNDEBUG -fPIC -fvisibility=hidden -fno-stack-protector -Os -ffunction-sections -fdata-sections -std=c++17 -MD -MT CMakeFiles/osrm_ext.dir/src/utility/osrm_utility.cpp.o -MF CMakeFiles/osrm_ext.dir/src/utility/osrm_utility.cpp.o.d -o CMakeFiles/osrm_ext.dir/src/utility/osrm_utility.cpp.o -c /py-osrm/src/utility/osrm_utility.cpp /py-osrm/src/utility/osrm_utility.cpp: In lambda function: /py-osrm/src/utility/osrm_utility.cpp:78:24: error: ‘struct osrm::engine::EngineConfig’ has no member named ‘default_radius’ 78 | config.default_radius = UNLIMITED; | ^~~~~~ /py-osrm/src/utility/osrm_utility.cpp:81:35: error: ‘struct osrm::engine::EngineConfig’ has no member named ‘default_radius’ 81 | assign_val(config.default_radius.get(), val); | ^~~~~~ [4/28] Building CXX object CMakeFiles/osrm_ext.dir/src/parameters/baseparameter_nb.cpp.o [5/28] Building CXX object CMakeFiles/osrm_ext.dir/src/parameters/routeparameter_nb.cpp.o [6/28] Building CXX object CMakeFiles/osrm_ext.dir/src/parameters/matchparameter_nb.cpp.o [7/28] Building CXX object CMakeFiles/osrm_ext.dir/src/osrm_nb.cpp.o ninja: build stopped: subcommand failed.

  *** CMake build failed
  [end of output]

note: This error originates from a subprocess, and is likely not a problem with pip. ERROR: Failed building wheel for osrm-bindings Failed to build osrm-bindings ERROR: Could not build wheels for osrm-bindings, which is required to install pyproject.toml-based projects

For reference, here's my dockerfile:

FROM ubuntu:22.04

RUN apt-get update

RUN apt-get -y install build-essential git cmake pkg-config libbz2-dev libxml2-dev libzip-dev libboost-all-dev lua5.2 liblua5.2-dev libtbb-dev

RUN git clone -b 5.27.1 --single-branch https://github.com/Project-OSRM/osrm-backend.git

RUN cd osrm-backend && mkdir -p build && cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && cmake --build . && cmake --build . --target install

RUN wget https://repo.anaconda.com/miniconda/Miniconda3-py38_22.11.1-1-Linux-x86_64.sh

RUN bash Miniconda3-py38_22.11.1-1-Linux-x86_64.sh -b -p /opt/conda

ENV PATH="${PATH}:/opt/conda/bin"
nilsnolde commented 1 year ago

OSRM doesn’t release that often anymore, we decided to go with master instead. I know @whytro had a PR changing smth about the default_radius property which definitely happened after 5.27.1 was released (and for the sake of the bindings, actually that must be why we decided to go with master). Try master, that should work, it’s what GitHub actions does.

Zebreu commented 1 year ago

Thanks, I could build it and test it out!

The route function seems about 5 times slower than what I'm already using (https://github.com/datakind/dk-routing/tree/main/dkroutingtool/c_binding_resources) across a test calculating 7000+ routes, so I'll probably keep using it since that's a bottleneck for us (and why I wrote the bindings in the first place). It's not a perfect comparison because my pybind11 bindings aren't compiled against the same osrm master but I wouldn't think that osrm got slower since 2019.

Do let me know if there's something easy performance-wise that I may be overlooking, I haven't looked at the whole source but this library is definitely more flexible and careful so I figure all those extra bits may be impacting performance.

Zebreu commented 1 year ago

I looked into the difference actually, I don't know if there's something to fix on py-osrm's end, but passing some variables into the route parameters as is will trigger a nanobind implicit conversion that fails (but still produces correct results in the end), and that eats up a lot of time.

The solution for me is to cast the coordinates:

coords = [(float(location1[1]), float(location1[0])), (float(location2[1]), float(location2[0]))]
route_params = osrm.RouteParameters(coordinates = coords, geometries='geojson', overview='simplified', annotations=[], steps=False)

If I do not use float() above, the same set of requests will take 37 seconds instead of 3, even if my values come from a float numpy array directly.

I'm happy to say it's now faster than my very simple bindings so I'll be able to modernize my repo thanks to py-osrm!

whytro commented 1 year ago

I'm glad to hear that you were able to get it working! Thank you for also looking into the root of the performance issues - it definitely seems worth investigating.

nilsnolde commented 1 year ago

Yeah that’s the kind of early testing we need:) thanks @Zebreu !

Though I’m confused, your coords aren’t float type then? If it’s just 53.74628 or so, it’s already a float and and explicit conversion would be a no-op.

Zebreu commented 1 year ago

That's why I was surprised!

1. coords = [(7.4252031, 43.7327806), (7.4239272, 43.7325243)] # Fast 
2. coords = [(location1[1], location1[0]), (location2[1], location2[0])] # Slow
3. coords = [(float(location1[1]), float(location1[0])), (float(location2[1]), float(location2[0]))] # As fast as 1

And location1 and location2 are directly coming from a numpy array:

locations = data[['lat','lon']].astype(np.float32).values # or float64 or left as is, all give the same slow implicit conversion warning
location1 = locations[0]
location2 = locations[1]
nilsnolde commented 1 year ago

Yeah I suspected there’s numpy involved. I don’t use it much, but now that’s a 32 bit float, it’s probably not a Python float. We’d have to make sure we’re detecting that and handle that on our side, or maybe there’s some nanobind magic we can enable. Using numpy here will be a very common case.

Probably it doesn’t do anything but can you try 64 bit numpy floats?

nilsnolde commented 1 year ago

Fwiw, if nanobind fails some conversion here, it really should fail the whole thing and be like „wtf is float32!?“, instead of silently decrease performance by 80%😳

Zebreu commented 1 year ago

A minimal example with float64 that's slow:

In [1]: import numpy as np
In [2]: import osrm
In [3]: py_osrm = osrm.OSRM(storage_config='/3wheeler/monaco-latest', use_shared_memory=False)
In [4]: a = np.zeros((2,2))
In [5]: a[0] = (7.41337, 43.72956)
In [6]: a[1] = (7.41546, 43.73077)
In [7]: route_params = osrm.RouteParameters(coordinates = [(a[0,0], a[0,1]), (a[1,0],a[1,1])])

nanobind: implicit conversion from type 'tuple' to type 'osrm.osrm_ext.Coordinate' failed!
nanobind: implicit conversion from type 'tuple' to type 'osrm.osrm_ext.Coordinate' failed!

I tried something else: image and crashed out of python