-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Distance support for table service #2764
Conversation
great job!!!! |
Awesome! |
@niemeier-PSI Nice! Do you by any chance know the amount of HD memory needed for the unpatched planet and Europe datasets? |
hey guys, I'm glad you like it! @RhinoDevel: I will check the memory requirements of v5.2.7 to be able to compare. Will take some time though as i first have to do the extraction/contraction for the unpatched version. |
@niemeier-PSI Thanks a lot!! Although I did not mean to put more work on your plate - just thought you might already know the numbers, anyway. |
@RhinoDevel no, it makes sense to have these numbers! Anyway, it's not much work for me, only for my computer ;) |
Ok, so here is the memory consumption on my system (--max-table-size 10000) for As posted above, the memory consumption for the table-distances branch rebased on tag v5.2.7 is All measurements performed after performing a quadratic table query involving 2500 locations in europe. |
Congratulations! :-) |
Thanks for making the effort of moving this upstream @niemeier-PSI! Clearly you understand the code base well. Great contribution! 👍 Sadly we can't accept this PR for the same reasons that we didn't accept previous contributions: It increases memory usage unconditionally, if you use the feature or not. I know this is however useful for a lot of people using OSRM for distance table computations, as such this is of course a very valuable contribution even if it does not live in the mainline. There are two approaches I can see to implement distances in table queries:
In terms of API design we could try to adapt your solution so it is compatible with the API that would result from #2399 making it easy to switch to an OSRM 6.0 based implementation. For this the following logic would need to be implemented:
|
What is not clear to me is why this can't be a build parameter? This way it Do I understand this correctly, by moving it to #2399 you 'misuse' the |
If you see how many parts of the code this touches, you will recognize that adding a build parameter will do nothing less then making an already hard to maintain code base even harder to maintain. The goal of #2399 is to return accurate duration values independent on what you use for routing. I see this an intended use. |
Hey TheMarex, thanks for you answer. The first point is clear to me, if the code is touched in to many ways, i understand that adding this will just make it more complex to maintain. |
@TheMarex : That is very sad. But please be so fair to admit that by adding the lane guidance support feature in 5.3, you where in a similar situation and decided otherwise. I do not need the lande guidance feature, however it is activated unconditionally and increases the memory consumption in more or less the same way as this branch does. I believe that simply rejecting a features just because it is not needed by everyone is not the best way to go. Instead one should think about changing the program architecture so that optional features can be activated/deactivated more easily (e.g. with command line parameters). Unfortunately both of your suggestions are are not suitable for us:
About your remark on the API: The reason why we made the output parameters optional is to keep the network traffic low: We do not want to increase the network traffic unconditionally , no matter if you use a feature or not. Actually, as our caller is a JAVA client, we seriously struggle parsing large JSON responses. Due to technical Java-VM limitations, the JSON response must not exceed 2GB |
That is a fair comparison. Indeed this is the unfortunate result for prioritization our own business needs. Honestly I would really like to change that, lane guidance for pedestrians does not make sense, but it is in there. I don't have a good solution here yet and it has not been a priority for us.
I'm open for ideas here. Apart from clustering the code base with compile-time switches I don't see a way to do this. Maybe some template programming would be the answer here? We basically have a similar problem for #2477. As you already experienced features like this are heavily intervened with core data structures. Part of the reason OSRM is so fast is that it sacrificed flexibility for speed. (to be fair some of the inflexibility is also the result of an organically growing code base)
Have you investigated using JNI? I think @danpat once experimented with writing bindings for libosrm. With big queries I usually advice to skip the HTTP API, as serialization and deserialization can add quite some overhead as you are experiencing.
I'm curious did you benchmark that? I would be very interested in some number, as I never got around to investigate this myself. |
@TheMarex Thank you for your reply. About the first one: Changes in the architecture to be more "optional feature friendly". I agree that littering the code base with compile-time switches is not a good idea. preprocessing steps (extraction mostly)
router
For my distances feature, it would mean that I would remove the distance data from the EdgeBasedGraph and the QueryEdge, and instead would make this accessible via the datafacade Second topic: alternatives for distance computation
I did not. There was a discussion on this approach in #1353 Third topic: API-issues
The problem is that in our architecture, the OSRM router is usually on a different machine than our software making the table request. For that reason, we are quite happy with the fact that you provide a fully functional webservice. Of course we could write another network service (using osrm as a library) running a proprietary protocol to overcome the limitations, but that would be one more component to maintain and it feels kind of redundant. On my shortlist on future contributions is the following approach: Add the option to respond with multipart HTTP responses. The JSON response would stay as before with one exception: Instead of having the table data as part of the JSON string, it would be transmitted binary in a binary part section of the HTTP response. |
What's the status here? |
You can get something similar if you change the |
@TheMarex many-to-many routing is based on durations, so in profile segment durations must be changed to distances. It is a little bit weired, but I think such values later must be completely abstracted from their names and use one value as a heap key and some other values as accumulated annotations if needed |
@dskrvk: Unfortunately, the status is unchanged, as there has been no response from the devs to my suggestions from August. I will use this oppourtunity to summarize what happened so far, and to raise questions (to the devs) on how to go on from this. Let me summarize what happened so far:
I am now wondering on how to move on. We plan to add additional features in the near future, which will build on this feature. This will make the diff even more complex and it will very quickly get to a point where a merge will get impossible, resulting in a permanent fork of OSRM for applications in transportation logistics. Imho it would be very sad if this is what would happen. You guys did an outstanding job with osrm, and I would rather like to contribute to help it become THE general purpose OSRM routing engine, than creating yet another variant for some niche applications. So my question to the devs is: What are your goals with the osrm project? If your answer tends towards the former, you have to provide an adequate architecture that can support optional features at no extra cost when turned off. It must be possible to integrate feature that are important to others, even if they are of no benefit to you. Otherwise OSRM cannot be THE osrm routing engine. As I already expressed repeatedly, I am happy to offer support on this. |
We've had to pay the additional memory price for the implementation of #2399. I think if we can do the following:
then we can implement this feature. I'm still hesitant to make it non-optional, but it's something we should have, this request comes up often. |
@danpat: I am very happy to hear this! Having the edge distances in a separate file and making it optional is a good idea. I will implement your suggestions and hope to see this feature merged afterwards. Then I can finally start working on the next items on our "wishlist". :) |
With the recent advances of min weight routing in place of duration come the need of getting duration and distance. |
@damu4 yes still the same response { |
@developerbhuv007 did you remove the previous make of osrm-routed before intalling this version https://github.com/niemeier-PSI/osrm-backend/tree/v5.2.7-psi you can remove by going into the build folder of previous install and typing below command removes all the files related to osrm then you do a fresh install
|
@sandeepgadhwal Thanks for the reply. I removed the whole build directory before installing the new version. |
link to repository forked by me : https://github.com/sandeepgadhwal/osrm-backend-distance it is the exact checked out version for distance tables. |
@sandeepgadhwal Thanks a lot. It is working fine now. Now I am getting following response when I am trying to find the matrix for more number of geo points. { Thanks in Advance |
Use osrn-routed with '--max-table-size' option to increase the limit.
…On Dec 14, 2017 10:42 AM, "Bhuvnesh" ***@***.***> wrote:
@sandeepgadhwal <https://github.com/sandeepgadhwal> Thanks a lot. It is
working fine now.
Now I am getting following response when I am trying to find the matrix
for more number of geo points.
{
message: "Too many table coordinates",
code: "TooBig",
}
Thanks in Advance
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2764 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABCIjAZWc0L36FQkS7El4gk197O3nt1Dks5tAN9wgaJpZM4Ji5at>
.
|
@developerbhuv007 was your problem solved by using |
@sandeepgadhwal yes it solved my problem. Thanks a lot @sashakh |
Is there a way to bring down the response size from
to
query what i am doing is |
@sandeepgadhwal Not with version 5.2.7. If I understand the API-Docs of vanilla ORSRM correctly (http://project-osrm.org/docs/v5.10.0/api/#general-options), using the option "generate_hints=false" would do the trick. But AFAIK the option is not supported in 5.2.7. |
@niemeier-PSI why they did not include such a wonderful feature in the official version, thanks for your effort and great contribution i hope they will include it in future as more and more people are interested in it. |
@sandeepgadhwal Thank you :) |
As for my case, I just need |
@hopewise AFAIK it is not possible to suppress the sources and destinations fields. |
Good day. I am trying to setup the OSRM that includes the distances and durations. It seems that i am unable to get the distances and duration in the table request. I have tried the following:
I also tried @sandeepgadhwal's post on Dec 14, 2017 and still got the same results. Am i missing something or doing something wrong? I have tried different versions and build for a few days now and still not getting the distances and durations back. |
The correct branch is table-distances. |
Thanks you! It's woriking now
…On 14 Jan 2018 13:26, "Gregor Rudolf" ***@***.***> wrote:
The correct branch is table-distances.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2764 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AfSlYs4Vd-lT0PvHpTAEZJDT1x9MsRKgks5tKeRSgaJpZM4Ji5at>
.
|
Don't know why this is not available in official version .
…On 16-Jan-2018 1:07 PM, "RhinoDevel" ***@***.***> wrote:
Just sayin'..
[image: 1]
<https://user-images.githubusercontent.com/5845035/34976735-3346d750-fa98-11e7-92e3-d794760c390d.png>
[image: 2]
<https://user-images.githubusercontent.com/5845035/34976746-3807c678-fa98-11e7-8c3c-fe69222b42cd.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2764 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARpk2J0XKBKjb3aJDlGHVQVILq3hsKq2ks5tLFGjgaJpZM4Ji5at>
.
|
Hello, With query that uses v1 as version, I am getting: { message : "Query string malformed close to position 93", code : "InvalidQuery"} |
any plans to merge this with the 5.12 version where the 'avoid' feature is an option? |
I would also like to vote for this feature to be merged into main. |
For anyone else who is wondering, this is still working great in 2018: Mem: 16432232 15017332 709708 2284 705192 1128032 The whole process took less than 12 hours to build and call time is excellent: time lynx --source 'http://127.0.0.1:5000/table/v1/driving/-117.1611,32.7157;-74.24,41.80;-78.51,43.02?sources=0&output_components=distances'|python -m json.tool >data 0.01user 0.00system 0:00.07elapsed 15%CPU (0avgtext+0avgdata 7188maxresident)k Even with 100 records response time is still under half a second: 0.01user 0.00system 0:00.20elapsed 5%CPU (0avgtext+0avgdata 7160maxresident)k |
This thread is hopeless... everybody else is already using niemeier-psi solution, and the admins here don't have the minimum sense of understanding the importance of all this... Congratulations again @niemeier-PSI , I have already cited you in two academic papers for your valuable (and not recognized here) contribution. |
For everyone else following this ticket, there is active work on this feature over in these two PRs: Compute durations at runtime: #4876 As stated earlier, we will not merge this PR. Once those two PRs are merged, we will probably close this one. The main blocker here was the additional memory usage, and dev bandwidth - @niemeier-PSI I'm sorry we didn't have the time/focus to bring this into the mainline after you made your changes in #2764 (comment). The two PR's above will implement distance response like so:
The approach looks promising - performance is only slightly impacted, overall memory consumption is reduced, and distances will become available in the response, working with all current features in OSRM (excludes/avoid, CH & MLD, etc). |
This is now available in OSRM 5.18 using the query options |
This pre-calculation (i.e. fast) functionality has been re-implemented in OSRM 5.20.0 as part of #5251 |
The "table" service now optionally provides distances next to driving durations.
Api-Changes:
QUERY: The table service accepts a new (optional) parameter
"output_components" to specify which data-components to output. The parameters value is a ';'-separated list of keywords. Currently the keywords "durations" and "distances" are supported with obvious meanings. In case the "output-components" parameter is not specified, the default value is "durations". This way the API is backward compatible.
RESPONSE: The "durations" output is provided only when requested as output component, the format is unchanged. If the output-component "distances" is requested, the response contains a "distances" field of the same format as "distances".
Code-Changes:
Status:
The current version is fully functional and already in use in a customized version. It is marked as "not-ready", because updates to the API-documentation are still missing. I am waiting for your approval on the changes before
updating the documentation.
Use-Case:
The algorithms of our supply-chain optimization software PSIglobal
(http://www.psilogistics.com/en/solutions/supply-chain-management/) require pairwise distances for a huge number of points (for freight cost calculations the optimizations are based on). The excellent performance of OSRM would make it a perfect provider for the distance matrices. However, obtaining distances instead of durations is crucial for us. This is why we customized OSRM accordingly, and would be very happy if this customization would find its way into the main branch.
Memory-Concerns:
We are running this customization (more precisely, this customization rebased on tag v.5.2.7) on planet dataset. Memory consumption of osrm-routed is about 68,5 GB.
(Option --max-table-size 10000 was used)
Memory consumption on europe dataset is about 24 GB