noi-techpark / odh-mentor-otp

4 stars 8 forks source link

A2.6 OTP geocoding mechanism adaptation #11

Closed rcavaliere closed 3 years ago

rcavaliere commented 3 years ago
rcavaliere commented 3 years ago

For the evaluation of the POI ODH integration, please consider these requests:

ghost commented 3 years ago

Analyzing the available datasources.. I noticed: Accomodation it's ok: https://tourism.opendatahub.bz.it/api/Accommodation?language=de&poitype=447&active=true&fields=Id,AccoDetail.de.Name,Latitude,Longitude&pagesize=10

But POI and ODHActivityPoi don't have required fields: Latitude,Longitude(for centering map on results) http://tourism.opendatahub.bz.it/api/Poi?language=de&poitype=447&active=true&fields=Id,Detail.de.Title,Latitude,Longitude&pagesize=200

ghost commented 3 years ago

mmh ok POI have a subfield named GPSInfo: http://tourism.opendatahub.bz.it/api/Poi?language=de&poitype=447&active=true&fields=Id,Detail.de.Title,GpsInfo&pagesize=200

rcavaliere commented 3 years ago

Yes, for ODHActivityPoi have a look at this: https://tourism.opendatahub.bz.it/api/ODHActivityPoi/smgpoi107?language=de

rcavaliere commented 3 years ago

For POI: https://tourism.opendatahub.bz.it/api/Poi/2379CD9828158D300CF0C677EF6431FA?language=de

rcavaliere commented 3 years ago

@bertolla will check with @RudiThoeni the correct functioning of the ODH Tourism API calls

bertolla commented 3 years ago

I deployed the new version of the otp bundle and made the geocoder available through https://geocoder.otp.opendatahub.testingmachine.eu/ OTP has the env var set GEOCODER_BASEURL=https://geocoder.otp.opendatahub.testingmachine.eu/ I'm not sure if it's working correctly and I don't see changes on http://journey.opendatahub.testingmachine.eu/ @stemove could you check if the geocoder is working as expected?

ghost commented 3 years ago

hi @bertolla

I see that the online web interface makes requests to localhost: Screenshot at 2020-12-22 10-37-46

each edits of config vars in the Journey service requires a rebuild of the image because the values of the vars are included in the javascript built for the application

probably only a journey rebuild is missing

ghost commented 3 years ago

geocoder deployed work fine: https://geocoder.otp.opendatahub.testingmachine.eu/v1/autocomplete?boundary.rect.min_lat=46.18828&boundary.rect.min_lon=10.470121&boundary.rect.max_lat=47.08878&boundary.rect.max_lon=12.255011&text=MERAN

text param must be 3 chars long configurable here

this request finds some results(in Pelias format)

// 20201222105748
// https://geocoder.otp.opendatahub.testingmachine.eu/v1/autocomplete?text=meran&boundary.rect.min_lat=46.18828&boundary.rect.min_lon=10.470121&boundary.rect.max_lat=47.08878&boundary.rect.max_lon=12.255011

{
  "geocoding": {
  "features": [
    {
      "properties": {
        "id": "1_it:22021:1332:0:8849",
        "gid": "opentripplanner:venue:1_it:22021:1332:0:8849",
        "layer": "venue",
        "source": "opentripplanner",
        "source_id": "1_it:22021:1332:0:8849",
        "name": "stop Merch ",
        ...
      }
    },
    {
      "properties": {
        "id": "1_it:22021:1115:0:8612",
        "gid": "opentripplanner:venue:1_it:22021:1115:0:8612",
        "layer": "venue",
        "source": "opentripplanner",
        "source_id": "1_it:22021:1115:0:8612",
        "name": "stop Zerun ",
       ...
      }
    },
    {
      "properties": {
        "id": "267E95077ED5C11308C24C97D045899C",
        "gid": "odh_accommodations:venue:267E95077ED5C11308C24C97D045899C",
        "layer": "venue",
        "source": "ODH_accommodations",
        "source_id": "267E95077ED5C11308C24C97D045899C",
        "name": "Apartment Merano",
       ...
      }
    },
    {
      "properties": {
        "id": "AAAC3456ED7EB4092D2BA9BD97F9AF47",
        "gid": "odh_accommodations:venue:AAAC3456ED7EB4092D2BA9BD97F9AF47",
        "layer": "venue",
        "source": "ODH_accommodations",
        "source_id": "AAAC3456ED7EB4092D2BA9BD97F9AF47",
        "name": "Apartments Meraner Inn",
       ...
      }
    },
    {
      "properties": {
        "id": "C32F3C6A45EA3F97803CE0FFB9CECA96",
        "gid": "odh_accommodations:venue:C32F3C6A45EA3F97803CE0FFB9CECA96",
        "layer": "venue",
        "source": "ODH_accommodations",
        "source_id": "C32F3C6A45EA3F97803CE0FFB9CECA96",
        "name": "Appartamento Centro Terme Merano",
        ...
      }
    },
    {
      "properties": {
        "id": "09780365F2A347683FCBBF96600D791B",
        "gid": "odh_pois:venue:09780365F2A347683FCBBF96600D791B",
        "layer": "venue",
        "source": "ODH_pois",
        "source_id": "09780365F2A347683FCBBF96600D791B",
        "name": "Agostini Meran",
        ...
      }
    },
    {
      "properties": {
        "id": "548506927A9A42DEABFC48965CF6BB07",
        "gid": "odh_pois:venue:548506927A9A42DEABFC48965CF6BB07",
        "layer": "venue",
        "source": "ODH_pois",
        "source_id": "548506927A9A42DEABFC48965CF6BB07",
        "name": "Alpinpool Maranza/Meransen",
        ...
      }
    },
    ...
  ],
  "bbox": [
    11.122026,
    46.5454335602364,
    11.7752977819131,
    46.820908
  ]
}
bertolla commented 3 years ago

Thx for the fast reply. I didn't have it as build argument and the url was missing the "/v1". I just redeployed, so it should work once the pipelines finishes.

rcavaliere commented 3 years ago

@bertolla @stemove good! I have tested the client and it seems to me that now the geocoder is correctly interrogated. However, the user experience is still not the expected one, if I type "hotel" I would expect a longer list of hotel coming from our Accomodation API. But probably this is related to the issues we discussed together last week in relation to filter search. Correct?

rcavaliere commented 3 years ago

Further details:

l'ordine e la quantita' di risultati e' configurabile, dal file:
https://github.com/noi-techpark/odh-mentor-otp/blob/development/geocoder/config.yml#L35
mentor-geocoder.png
modificando l'ordine in cui si trovano le sezioni nel file yaml: opentripplanner, accommodations, pois ecc
viene modificato anche l'ordine in cui viene mostrato il risultato dall'alto verso il basso, quindi di default per primo opentripplanner(cioe' gli stop e gli incroci indicizzati dalla istanza otp)

il parametro size per ogni sezione indica il massimo di risultati visualizzabili per ogni singolo entrypoint, ho dovuto usare 3 per cercare di ottimizzare i tempi di risposta per i motivi che sapete.
quindi per ottenere il tipo di risultato atteso descritto da Roberto dovreste spostare in alto la sezione accomodations ed aumentare il size ad esempio a 8

Invece il totale di risultati mostrati credo sia purtroppo per ora non configurabile in quanto hardcoded nel codice della libreria di front end che abbiamo utilizzato.
rcavaliere commented 3 years ago

The geocoding feature is unfortunately not so performant at the moment. We should probably discuss with @RudiThoeni Can we test the STA filter also here?

bertolla commented 3 years ago

There is no STA filter. STA gets a cached json off all data once a day. Rudi also told me that currenty 3 api calls are done by the geocoder but only 2 are needed since one is just a query on the smaller dataset. I'm also not sure if the API is the bottleneck since the db calls now are fast. 15ms per query. We can also increase the result set if needed. How many results do you expect to see? Currently it's 5, which imao is already good since the user can filter more.

bertolla commented 3 years ago

I removed the superfluous call and noticed that the pagesize filter of the accomadation call is not working as expected, it always returns all: https://tourism.opendatahub.bz.it/api/Accommodation?language=de&poitype=447&active=true&fields=Id,AccoDetail.de.Name,AccoDetail.de.City&pagesize=5&searchfilter=Hotel Maybe @RudiThoeni can fix this as soon as he's back.

rcavaliere commented 3 years ago

@bertolla @RudiThoeni we will have to check this together in our next sprint

RudiThoeni commented 3 years ago

ok i will check it ;) i think it has to do with a hardcoded rule for the sta call

RudiThoeni commented 3 years ago

ok should be fixed now.... there was a simple if that identifies this call as call from sta, i have to improve here.....

bertolla commented 3 years ago

Filter works now, but the call is still slow. You mean that with "improve"? image

bertolla commented 3 years ago

@stefanocudini I noticed that the web client starts a request every time one types. Is there not a way to improve this behavior? Like described here: https://stackoverflow.com/questions/4220126/run-javascript-function-when-user-finishes-typing-instead-of-on-key-up

stefanocudini commented 3 years ago

@bertolla yes sure! we can modify the frontend to add a debounce function, at this moment this behavior is not supported by the library used for the frontend, it is definitely one of the things to improve, along with the count of result rows. I have marked among the small todos

RudiThoeni commented 3 years ago

@bertolla now it should be fast..... i have to remove the gin index over the whole json otherwise the query planner is unable to take the search index......... not the solution but now its working better

bertolla commented 3 years ago

The api calls are super fast now. The geocoder itself sometimes goes in timeout if the data is not as expected like for example I got this exception searching for Dant:

[GEOCODER] search: "Dant" parallel remote requests...
(node:1) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'Latitude' of undefined
    at /home/formatters.js:127:40
    at arrayMap (/home/node_modules/lodash/lodash.js:639:23)
    at Function.map (/home/node_modules/lodash/lodash.js:9580:14)
    at Object.ODHActivityPoi (/home/formatters.js:123:12)
    at /home/index.js:146:36
    at Array.forEach (<anonymous>)
    at /home/index.js:142:12
    at /home/node_modules/parallel-http-request/src/parallelrequest.js:394:13
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
(node:1) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:1) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
2021-02-24T12:38:25.196Z - error: [api] elasticsearch error Error: Request Timeout after 120000ms

This case should be handled by the geocoder. I also think, I'm not sure about that, that you also need to pass Longitude and Latitude in fields so that geocoder gets that information.

rcavaliere commented 3 years ago

@RudiThoeni I am not sure that the API provides the same results as for the web application of STA. E.g. if a search "hotel Laurin" I get "39100 Bolzano Bozen, Restaurant Laurin in the Parkhotel Laurin" as a result on suedtirolmobil, whereas I don't get results on this prototype application. Do you have a reason why we have this behaviour? I would expect to get results...

RudiThoeni commented 3 years ago

there is a big difference STA loads a json with All Accommodations and is doing the search by their implemented logic in javascript on the complete Json

The Api call here uses a searchfilter and the api directly filters however if i pass hotel Laurin in the searchfilter i get results from the api, try this link........ https://tourism.opendatahub.bz.it/api/Accommodation?language=de&poitype=447&active=true&fields=Id,AccoDetail.de.Name,AccoDetail.de.City&pagesize=5&searchfilter=hotel%20Laurin

rcavaliere commented 3 years ago

Ah ok, thanks for this detail. @stefanocudini do you have an idea why the client does not show the expected result in this specific case?

bertolla commented 3 years ago

This is the current configuration for the geocoder : https://github.com/noi-techpark/odh-mentor-otp/blob/c7ec36ca321672b957c2c523561b27bc2ce985e2/geocoder/config.yml#L22 I think it uses multiple sources but I don't get the prioritisation.

stefanocudini commented 3 years ago

I'm testing this and sending you a PR soon to resolve

stefanocudini commented 3 years ago

This case should be handled by the geocoder. I also think, I'm not sure about that, that you also need to pass Longitude and Latitude in fields so that geocoder gets that information.

this PR https://github.com/noi-techpark/odh-mentor-otp/pull/32 solve the above bug..

stefanocudini commented 3 years ago

Ah ok, thanks for this detail. @stefanocudini do you have an idea why the client does not show the expected result in this specific case?

about this.. I found that the problem is not in the geocoder module but in the client side library.. when searched text contains spaces. I will look for a solution soon

stefanocudini commented 3 years ago

This is the current configuration for the geocoder : https://github.com/noi-techpark/odh-mentor-otp/blob/c7ec36ca321672b957c2c523561b27bc2ce985e2/geocoder/config.yml#L22

I think it uses multiple sources but I don't get the prioritisation.

@bertolla I'm testing locally and it works, I realized that some environment variables were missing in your part of the infrastructure, I inserted them here, (just included in the next PR): https://github.com/openmove/odh-mentor-otp/blob/development/infrastructure/docker-compose.run.execute.yml#L44 but you should verify that it is also placed in your other infrastructure files

in a few minutes I will send a new PR that fixes some of the problems described in this issue

rcavaliere commented 3 years ago

Very good @stefanocudini! I think the issue is closed, the geocoder is working now as expected. Let's close this issue and in case of some further strange behaviours let's open dedicated issues

stefanocudini commented 3 years ago

@rcavaliere @bertolla thanks for the quick feedback

here are some of the minor issues I had marked so far about geocoder UI behavior https://github.com/openmove/odh-mentor-otp/issues?q=is%3Aissue+is%3Aopen+label%3Ageocoder

in the next PRs I'll try to solve everything

bertolla commented 3 years ago

@bertolla I'm testing locally and it works, I realized that some environment variables were missing in your part of the infrastructure

About this, I might have missed them when I set it up. I did not found this environment variables in the documentation and thought they would only be needed when building the images. Would it be an idea to describe for each container/service, the building args and the environment variables at runtime?

rcavaliere commented 3 years ago

@bertolla @stefanocudini yes please, let's document everything needed!

stefanocudini commented 3 years ago

About this, I might have missed them when I set it up. I did not found this environment variables in the documentation and thought they would only be needed when building the images. Would it be an idea to describe for each container/service, the building args and the environment variables at runtime?

@bertolla absolutely good idea!

I'm updated documentation now it should be clearer, divided by container as Patrick suggestion. https://github.com/openmove/odh-mentor-otp/tree/development#docker-environment

Build arguments need only for journey because it is a client module, all the others containers almost exclusively use environment variables used at run time.

Look at this new PR: https://github.com/noi-techpark/odh-mentor-otp/pull/36