Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic costing_options in request to Valhalla #903

Open
DaanAug opened this issue Mar 17, 2023 · 6 comments
Open

Dynamic costing_options in request to Valhalla #903

DaanAug opened this issue Mar 17, 2023 · 6 comments

Comments

@DaanAug
Copy link

DaanAug commented Mar 17, 2023

Hi everyone,

Does anyone know of an option to include the costing_options parameter to the Valhalla service? Currently what i think happens is that the profile gets used to define the costing value for the Valhalla request. There is no way to send the costing_options to the matrix or route endpoint of Valhalla.

I have noticed there is a branch named experiment/extra-valhalla-options but the values for costing options are hard-coded.

Did anyone ever figure out a way to add extra costing_options dynamically? Perhaps with extra custom profiles?

Thanks in advance!

PS: I know there are also a few related issues asking the same thing, but I was wondering if anyone has found out a solution.
(#584 #601 VROOM-Project/vroom-express#45)

@nilsnolde
Copy link
Contributor

There's no solution other than adding it to VROOM source code, in case we'd like to be able to pass costing options directly (which I'd highly prefer too!). You still can use it of course, you'll have to call Valhalla separately and use VROOM's custom matrix input (you probably read that as well).

I'm still very open to get this funded and I'm sure @jcoupey would be ok to include it in master (with my promise to maintain it). It'd really make things a lot more convenient. I might have the opportunity soon to work on this later this year on a client's project. I'll update before of course.

@jcoupey
Copy link
Collaborator

jcoupey commented Mar 20, 2023

I totally get how this would add ease of use, especially when working with per-request configurable routing engines such as Valhalla. What is not totally clear for me as of now is how to add this kind of stuff in a clean and generic way. Frankly, I expect that as we start digging into this, there is virtually no end to what users will want to be able to configure. Examples of not-so trivial questions:

  • we interface with several open-source routing engines, how do we handle the fact that some options are common while others are router-specific?
  • what is the granularity of vehicle-related options (profile, vehicle)?
  • what about task-related option (think approaches), how do we add those without bloating the API?
  • do we even always want to allow user routing configuration?

@nilsnolde
Copy link
Contributor

nilsnolde commented Apr 20, 2023

The main problem in my mind is that OSRM is the exception to the other routing engines, who all support POST request with JSON payloads. That'd be fairly easy with rapidjson, we could just let a user define the usual JSON payload, ignore the locations & profile (as that's set another way in VROOM IIRC) and pass the rest of the query as-is to the routing engines, who'll also deal with throwing in case of invalid input. That'd also make it future-proof. (EDIT: route requests are optional anyways, either we'd not allow/respect a parameterization or take the same one as for matrix requests)

I'm not sure how we'd deal with that for OSRM though. I never liked their request schema, for me it's a bit "too correct", it seems to try to be RESTful when it really can't be. Maybe it's historical reasons, but JSON payloads are just so much easier to handle (client-side). Also not sure what's necessary to add support for that in OSRM upstream . I know there's been Project-OSRM/osrm-backend#6294, so I might brainstrom with Siarhei a bit, who seems to have it solved for his private instance. libosrm could also have a wrapper which accepts the same JSON instead of building the "request" via its object API (Valhalla does the same).

Seeing that it might take more effort to enable similar behavior for OSRM, would you generally be ok if we had such JSON input support for Valhalla to begin with? TBH, both ORS & OSRM have very little request configuration to offer for matrix computations, so it's less "urgent" IMO. The setup should of course be generic enough so the other routers can catch up.

We can have a chat around the topic in a meeting if you want. It needs quite a lot of context which is easier communicated 1:1 instead of Github:)

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 27, 2023

@nilsnolde yes, let's sync on that, feel free to ping me directly for a call.

@nilsnolde
Copy link
Contributor

nilsnolde commented May 3, 2023

Just keeping our thoughts here:

  • for the most part we'll "only" add "costing options" for vehicles, less so for jobs, though there might be some notable exceptions, e.g. approaches (OSRM), preferred_side (Valhalla) which we could specify on shipment/job individually
  • introduce a vehicle_options top-level key, which holds an array of "payload" objects for the routing engines. Any mentions of "profile"/"costing" or "locations" will be ignored as those are set directly in the VROOM API.
  • all vehicle_options are valid for both the matrix & routing requests
  • also introduce a profiles array where the position of a profile in this array relates to the position in the vehicle_options array
  • as OSRM has no POST JSON endpoint (yet), we'll need to add some code that lets us specify "fake" JSON for OSRM's (admittedly few) matrix/route options; also don't forget about libosrm

I think that was it, right @jcoupey ?

Again, anyone having enough interest in this to fund the whole or part of the development, please reach out. Otherwise I'm happy to have a plan to be able to move forward once this should have some priority on my side.

@jcoupey
Copy link
Collaborator

jcoupey commented May 5, 2023

That's pretty much it, yes!

On the format, relying on having the same rank for matching options/profiles in different arrays does not sound that convenient. We could instead have profile values as key in a routing_options object, just as we allow providing custom matrices:

{
  "routing_options": {
    "car": {
      ...
      },
    "bike": {
      ...
      }
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants