Move ProfileResolver out of GraphHopper class #1958

easbar · 2020-03-17T19:13:49Z

In this PR I moved the ProfileResolver out of GraphHopper and into the Route/Isochrone/SPTResource. This means:

GraphHopper#route only considers the profile parameter (instead of vehicle,weighting,edge_based). At the moment it even throws an error if these parameters are set. Is that ok or should we go for a 'softer' migration and change this to an error log only? A missing profile parameter is definitely an error, I just wonder if its too strict to throw an error in case there is also vehicle/weighting (which are not used anyway).
To make use of the profile parameter we now have to add routing profiles also for non-prepared (no CH or LM) algorithms. GH configs without profiles lead to an error with this PR!
ProfileResolver does the conversion of the vehicle/weighting/edge_based parameters and the profile name is passed to GraphHopper. Therefore the actual behavior of the GH server should not be changed (unless there is a bug).

Another change that is done here is that we no longer use the first flag encoder as default vehicle. We already did this for CH before, but now its the same for CH/LM/flex. If the vehicle parameter is not set (and ch.disable=true) we are trying to find a matching profile based on the other parameters. If there are multiple matches there will now be an error (previously we would have used the first encoder as default vehicle).

I think it might be useful to merge these changes before in #1859 we:

Actually expose the new profile parameter via the web api. We probably also want to adjust the integration tests then and only keep a few that test the parameter conversion (profile selection). The good thing with this PR is that we can check if the conversion works before changing the tests etc.
Implement the merging rules between profile parameters and hints (e.g. take distance factor for short_fastest weighting from profile, but overwrite if given in request hints and not CH etc.)

Current todos for this PR:

adjust config-example.yml
add integration tests for edge_based/turn_costs parameter (and the corresponding conversion)/what about turn_cost=true/false cross querying for LM?
merge profile and request hints

…r instead, many tests failing

…, use profiles for non-prepared too

easbar · 2020-03-17T19:14:56Z

api/src/main/java/com/graphhopper/GHRequest.java

@@ -34,6 +34,9 @@
 */
 public class GHRequest {
    private List<GHPoint> points;
+    // todonow: keep this here or put it into hints, and even more important: can we remove vehicle+weighting from
+    // hints?
+    private String profile = "";


I think we discussed this in #1859 already: We'll def. remove vehicle+weighting from the HintsMap, but maybe in a follow-up PR? Since the profile parameter will be required it makes sense to keep it as a separate field?

Yes, I would also use it here and remove weighting and vehicle in a later PR

easbar · 2020-03-17T19:16:39Z

core/src/main/java/com/graphhopper/GraphHopper.java

@@ -824,6 +809,9 @@ public boolean load(String graphHopperFolder) {
    }

    private void checkProfilesConsistency() {
+        if (profilesByName.isEmpty()) {


There are a few odd cases where we do not use GraphHopper for routing (like NearestResource) so these do not necessarily require profiles. Not sure, to me this seems rather the exception so I am checking the existence of at least one profile here. Once we do #1901 we can move this check into GraphHopperRouter's constructor for example.

easbar · 2020-03-17T19:18:58Z

core/src/main/java/com/graphhopper/GraphHopper.java

+            // todonow: maybe still allow something like running a (non CH) profile edge-based or not (if no turn costs or something)?, also see traversal mode below
+            if (!request.getHints().get(Routing.EDGE_BASED, "").isEmpty())
+                throw new IllegalArgumentException("GHRequest may no longer contain the edge_based=true/false parameter, use the profile parameter instead, see #todonow");
+            // todonow: do not allow things like short_fastest.distance_factor or u_turn_costs unless CH is disabled and only under certain conditions for LM


This is about the 'merging' feature where some properties are set right in the profile (like distance_factor), but they might be overwritten per request. The problem is that this does not work with CH (and only some special cases work for LM), so I am a bit unsure where to put these checks. They cannot really go into createWeighting (where the merging will happen?!), because in this method we do not 'know' whether the weighting is created for CH or not...

for now we are just blindly merging the hints, later we will add these kind of checks here to be more strict

easbar · 2020-03-17T19:20:39Z

core/src/main/java/com/graphhopper/GraphHopper.java

            if (!profile.isTurnCosts() && !request.getCurbsides().isEmpty())
                throw new IllegalArgumentException("To make use of the " + CURBSIDE + " parameter you need to use a profile that supports turn costs");

+            // todonow: should we be able to control this using the edge_based parameter?


Maybe we can do this later, see #1702.

easbar · 2020-03-17T19:23:24Z

core/src/main/java/com/graphhopper/routing/ProfileResolver.java

+        Boolean edgeBased = getEdgeBased(hintsMap);
+        Integer uTurnCosts = getUTurnCosts(hintsMap);
+        return (edgeBased == null || p.isTurnCosts() == edgeBased) &&
+                (uTurnCosts == null || uTurnCosts.equals(getUTurnCosts(p.getHints()))) &&


Since we are now selecting a CHProfileConfig instead of a CHProfile we can deal with the case of multiple edge-based profiles with different u_turn_costs again (c.f. #1949). This should also help to get rid of Weighting#getFlagEncoder in the long run.

easbar · 2020-03-17T19:26:26Z

docs/web/api-doc.md

@@ -71,6 +71,7 @@ empty list for `profiles_ch`.
 Parameter        | Default    | Description
 :----------------|:-----------|:-----------
 ch.disable       | `false`    | Use this parameter in combination with one or more parameters of this table
+# todonow: update docs regarding profile etc.


This file is supposed to describe the web-api, but this documentation also is available on the directions-api documentation website. Does it make sense to keep this updated here? Or should this be rather about the documentation of the parameters of the open source server?

Yes, definitely this is different. This doc should should show all the features, e.g. including public transit and mvt endpoint etc that we do not have in the GraphHopper Directions API.

Ok so this file is actually the documentation of the open source server then? I will try to make this a bit more clear and for the hosted service point to the online docs then?

I wanted to avoid doing "advertisements" here, but probably you are right and this would be more clear.

easbar · 2020-03-17T19:28:57Z

tools/src/main/java/com/graphhopper/tools/Measurement.java

@@ -73,6 +75,7 @@
    private final Map<String, Object> properties = new TreeMap<>();
    private long seed;
    private int maxNode;
+    private String vehicle;


Here I thought the measurements are running only for one vehicle anyway (and if we want to test others we might as well start the measurement again for the other vehicle), so it makes sense to keep the vehicle as a class variable (just like weighting) rather than pass it with every querysetting?

In the CustomWeighting branch I already use profile and then I do not want the vehicle in the request: https://github.com/graphhopper/graphhopper/pull/1841/files#diff-cd749cc545f8b84137c2b75b2d174b72 but probably this can be still done somehow.

Ok yes vehicle and weihgting should be mostly removed from measurement and only profile should be left then?

Yes, but let's do this later :)

There isn't really anything to do. vehicle&weighting are only used to create the profile initially.

# Conflicts: # core/src/main/java/com/graphhopper/routing/ProfileResolver.java # core/src/test/java/com/graphhopper/routing/RoutingAlgorithmIT.java # reader-osm/src/test/java/com/graphhopper/GraphHopperIT.java # reader-osm/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java # tools/src/main/java/com/graphhopper/tools/CHMeasurement.java # tools/src/main/java/com/graphhopper/tools/Measurement.java # web-bundle/src/main/java/com/graphhopper/resources/IsochroneResource.java # web-bundle/src/main/java/com/graphhopper/resources/SPTResource.java

# Conflicts: # core/src/main/java/com/graphhopper/routing/ProfileResolver.java # reader-gtfs/src/test/java/com/graphhopper/AnotherAgencyIT.java # reader-gtfs/src/test/java/com/graphhopper/ExtendedRouteTypeIT.java # reader-gtfs/src/test/java/com/graphhopper/GraphHopperGtfsIT.java # reader-gtfs/src/test/java/com/graphhopper/GraphHopperMultimodalIT.java # reader-gtfs/src/test/java/com/graphhopper/RealtimeIT.java # reader-osm/src/test/java/com/graphhopper/GraphHopperIT.java # reader-osm/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java # web/src/test/java/com/graphhopper/http/SpatialRulesTest.java # web/src/test/java/com/graphhopper/http/resources/ChangeGraphResourceTest.java # web/src/test/java/com/graphhopper/http/resources/GtfsTest.java # web/src/test/java/com/graphhopper/http/resources/I18nResourceTest.java # web/src/test/java/com/graphhopper/http/resources/IsochroneResourceTest.java # web/src/test/java/com/graphhopper/http/resources/MvtResourceTest.java # web/src/test/java/com/graphhopper/http/resources/NearestResourceTest.java # web/src/test/java/com/graphhopper/http/resources/NearestResourceWithEleTest.java # web/src/test/java/com/graphhopper/http/resources/PtIsochroneTest.java # web/src/test/java/com/graphhopper/http/resources/RouteResourceWithEleTest.java # web/src/test/java/com/graphhopper/http/resources/SPTResourceTest.java

easbar · 2020-03-20T06:59:34Z

core/src/main/java/com/graphhopper/GraphHopper.java

+            // Merge profile hints with request hints, the request hints take precedence.
+            // Note that so far we do not check if overwriting the profile hints actually works with the preparation
+            // for LM/CH. Later we should also limit the number of parameters that can be used to modify the profile.
+            PMap hints = new PMap();
+            hints.putAll(profile.getHints());
+            hints.putAll(requestHints);


These are (the very simple) rules how we merge the profile hints with the request hints. The only thing I am wondering now is how we do this for block_area? Shall we do this in a similar way (use hints from profile and overwrite with request hints)? Do we need this here or is it ok to only create block areas from request hints (not the profile) here, and add this in #1776? Its still a bit unfortunate that we do not do the block_area stuff inside createWeighting. Now it looks all we are missing to do this in createWeighting are the points?

The only thing I am wondering now is how we do this for block_area?

Hmmh, we would need to add it to the list of shapes

Do we need this here or is it ok to only create block areas from request hints (not the profile) here

I would postpone this and properly support this in CustomWeighting.

Now it looks all we are missing to do this in createWeighting are the points

Yeah, although we only need this because of the is-in-area check which we could keep out of the Weighting creation somehow.

# Conflicts: # core/src/main/java/com/graphhopper/routing/ProfileResolver.java # tools/src/main/java/com/graphhopper/tools/Measurement.java

# Conflicts: # core/src/main/java/com/graphhopper/routing/ProfileResolver.java # web/src/test/java/com/graphhopper/http/resources/RouteResourceProfileSelectionTest.java

easbar added 9 commits March 17, 2020 18:56

Throw error if request has weighting or vehicle, use profile paramete…

1f890aa

…r instead, many tests failing

Fix some of the failing tests

b461512

Add a few more checks

aeffb34

Move profile resolver from GraphHopper to Route/Isochrone/SPTResource…

a021160

…, use profiles for non-prepared too

Minor

4267ea5

Add/move some profile resolving tests

aedcc4d

Minor fixes

9a43eef

Fix test

8d1eeda

Remove vehicle/weighting parameters in measurement

e3e628b

easbar added the refactoring label Mar 17, 2020

easbar added this to the 1.0 milestone Mar 17, 2020

easbar commented Mar 17, 2020

View reviewed changes

easbar added 5 commits March 17, 2020 20:46

Add todo to respect profile parameter later

13c02ea

Add integration test for edge_based/turn_costs parameters

18d117a

Fix formatting in Measurement.java

2c97604

Fix measurement: Add LM profiles with and without turncosts.

ff55558

Add javadocs for ProfileResolver

3751974

karussell mentioned this pull request Mar 18, 2020

refactor PMap to use string,object instead of string,string #1956

Merged

easbar added 4 commits March 18, 2020 12:16

Replace hints.get(EDGE_BASED, "") with hints.has(EDGE_BASED)

c9c7e36

Fixes after merge

96fec57

Update api-doc

12f06fb

easbar mentioned this pull request Mar 19, 2020

more PMap refactoring #1962

Merged

easbar added 7 commits March 19, 2020 12:18

minor

7a0844b

Strictly separate profiles with/without turn costs also for LM

df15f83

Update todos

8d57c53

Update issue references and todos

757cc1d

Merge profile hints with request hints before creating the weighting.

4c770ec

prepare merge

cb7b451

easbar added 2 commits March 19, 2020 19:59

minor

95d5ed2

Fix test after merging master

e774c1c

easbar commented Mar 20, 2020

View reviewed changes

Merge branch 'master' into profile_parameter2

8696944

easbar mentioned this pull request Mar 20, 2020

Use new profile parameter to select routing profile #1934

Merged

1 task

easbar added 10 commits March 21, 2020 10:20

Merge branch 'master' into profile_parameter2

ac9280f

# Conflicts: # core/src/main/java/com/graphhopper/routing/ProfileResolver.java # tools/src/main/java/com/graphhopper/tools/Measurement.java

Adjust after merging master, but test still fails

1427822

Pick another test from master

00c74ab

Pick recent changes from master, remove default_vehicle=encoders[0] rule

48c997b

Remove default vehicle for GET requests from RouteResource

4d60cfb

Update profile selection test

f3c63e9

Merge branch 'master' into profile_parameter2

e55c94d

# Conflicts: # core/src/main/java/com/graphhopper/routing/ProfileResolver.java # web/src/test/java/com/graphhopper/http/resources/RouteResourceProfileSelectionTest.java

Remove todos

50116c5

Some minor cleanups

019beb4

Add comments

116a864

easbar merged commit 6daf068 into master Mar 23, 2020

easbar deleted the profile_parameter2 branch March 23, 2020 20:45

easbar mentioned this pull request Mar 26, 2020

turn_costs handling in /isochrone #1967

Closed

2 tasks

NoamDev mentioned this pull request May 11, 2020

docs/core/routing.md documentation is outdated #2030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ProfileResolver out of GraphHopper class #1958

Move ProfileResolver out of GraphHopper class #1958

easbar commented Mar 17, 2020 •

edited

easbar Mar 17, 2020

karussell Mar 18, 2020

easbar Mar 17, 2020

easbar Mar 17, 2020

easbar Mar 19, 2020

easbar Mar 17, 2020

easbar Mar 17, 2020

easbar Mar 17, 2020

karussell Mar 18, 2020

easbar Mar 18, 2020

karussell Mar 18, 2020 •

edited

easbar Mar 17, 2020

karussell Mar 20, 2020

easbar Mar 20, 2020

karussell Mar 20, 2020

easbar Mar 23, 2020

easbar Mar 20, 2020 •

edited

karussell Mar 20, 2020

Move ProfileResolver out of GraphHopper class #1958

Move ProfileResolver out of GraphHopper class #1958

Conversation

easbar commented Mar 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell Mar 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar Mar 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar commented Mar 17, 2020 •

edited

karussell Mar 18, 2020 •

edited

easbar Mar 20, 2020 •

edited