Making languages dynamic with command arguments #125

Svantulden · 2014-12-10T10:19:17Z

Heya!

Thanks for this amazing library! Photon is best suited for our geocoding autocomplete needs compared to Nominatim (which we still use for reverse-geocoding and details).

I made some changes to allow for dynamic languages in Photon. We specifically needed to add Dutch as a language, and while I went over the language-specific code in Photon, I noticed that it wouldn't be too hard to make it fully dynamic.

The way it works now is as follows:

To specify languages that should be imported during nominatim-import

Use the nominatim-import-languages command argument. i.e.
java -jar photon-0.1.2.jar -nominatim-import -nominatim-import-languages nl,en -host localhost -port 5432 -database nominatim -user user -password password
for importing only the default, en and nl tags.

This could also be handy to keep Photon data as small as possible without languages that you are sure you are not going to use.

To specify languages for run-time

This could maybe be handy to block languages/enable languages for the request handler. Use the supported-languages command argument. i.e.
java -jar photon-0.1.2.jar -supported-languages nl,en
to use the nl and en language tags (defaults to en when no lang is specified).

However...
When testing this functionality, I noticed that the order of results was different between lang=en and lang=nl. After some debugging I noticed that it was the same on all the previous enabled languages (en,fr,it,de), but different on the nl tag.

I found out this was because I didn't update the mappings.json which is used on import, to specify collectors for languages. This should be dynamically changed too, when specifying nominatim-import-languages, which leaves this PR not finished.

Thoughts?

Thoughts & discussions are very welcome, on the code I wrote and on how I/we could tackle this problem correctly.

Things to do:

Make mappings.json dynamic somehow
Fully test the changes
How would we handle the problem when supported-languages has specified a language that doesn't exist in the data

PS. I also added specific Dutch postcode checking to this PR, which may help other Dutch users of Photon. This filters out double highway names who are part of the same street. In The Netherlands, a street can have different postcode letters (i.e. 6532RA and 6532BE could be the same street). The numbers however, stay the same. So to avoid clutter, I filter results that have the same postcode number AND name.

- Use -nominatim-import-languages command with comma separated languages to load those languages during import - Use -nominatim-import-languages command with comma seperated languages to use certain languages during run Example: -nominatim-import-languages en,de,nl

Use -nominatim-import-languages for importing and -supported-languages for runtime languages

- Filter doubles out if there are highways with the same name & postcode numbers when language is 'nl' (i.e. the same street has 6532RA and 6532BE, which should only be returned once to prevent clutter in autocomplete)

yohanboniface · 2014-12-12T11:36:47Z

Very nice move @Svantulden! This really is a key point to make Photon more suitable.

About the languages currently hard coded in the mappings, I think this is a bad thing, that we only made to make our life easier until now.

What would be the best move for this, imho, is to make the mappings.json a template (renaming it accordingly, like mappings_template.json just to make it clear), then to load it in Java, and itering to add the de, fr, etc. properties where needed, according to the activated languages.

Things will become a bit harder the day we'll have some settings specific by language (so for example an analyser specific), but certainly not a blocker I think.

I'm not the Java guy of the team, so I will let others review the code in details.

@christophlingg @karussell thoughts on this?

yohanboniface · 2014-12-12T11:38:35Z

es/query_location_bias.json

@@ -67,8 +67,8 @@
            "script_score": {
              "script": "dist = doc['coordinate'].distanceInKm(lat, lon); 0.5 + ( 1.5 / (1.0 + dist/40.0) )",
              "params": {
-                "lat": ${lat},
-                "lon": ${lon}
+                "lat": "${lat}",


This will cast floats to strings, not sure we really want to do that. What was your rationale here (apart from making the json a vaild one ;) )?

Woops! That is an oversight on my part.. Did not realize it would be casted as string. Will revert this change

christophlingg · 2014-12-12T13:17:35Z

Very nice @Svantulden ! A lot of people will benefit from your changes. Just some weeks ago someone contacted me for Hebrew support.

As for the mapping: I would also go for the solution @yohanboniface described. Until know I paid attention that our small python based scripts for prototyping can still use the same mapping file. Probably this won't be possible any more, but it should not be that bad.

Great work!

Svantulden · 2014-12-15T07:28:48Z

Thanks alot for reviewing my PR, @yohanboniface & @christophlingg ! Nice to hear that it is in the right direction for Photon.

I will experiment some with using a json template for the mappings.json. I don't have much experience with that, but maybe I can do it about the same as the query_location_bias.json.

yohanboniface · 2014-12-15T09:28:05Z

I don't have much experience with that, but maybe I can do it about the same as the
query_location_bias.json.

Humm, this could work, but I don't think this is the best approach, given that we'll need a more grained workflow (create the lang collectors, add some copy_to here and there, add some lang fields…): imho, you should instead parse the json to have a java structure you can work on, and then dump it again to json (or is there an ES API that takes a JSONObject as settings instead of a json string? I don't know).

- Should be submitted as separate PR

Svantulden · 2014-12-15T15:42:24Z

Hey @yohanboniface & @christophlingg, I did some work on the parsing of the (empty of lang specific code) mappings.json.

It's in a pretty rough form now, I would appreciate some feedback on how we can make this smoother. How it works now:

The mappings.json file is stripped of all lang-specific strings and objects
I iterate over the selected langs, and add JSONObjects to the corresponding fields that need language specific objects
I have three predefined json objects as strings with {lang} to be replaced for each lang
For the default property of name, we add the lang specific collector

This works pretty okay, but what I am worried about is that it is going to be a bit of work when Photon gets updated with a new or changed mappings scheme. Then you need to update all the hooks too. Also, the code seems a bit overly duplicated.

Apart from that, I fixed the following:

Reverted the NL postcode check
Removed some spaces
Unified the commandline param to using-languages. What do you think about this name?
Reverted the valid json

yohanboniface · 2014-12-15T15:54:30Z

src/main/java/de/komoot/photon/importer/CommandLineArgs.java

@@ -16,6 +16,9 @@

 	@Parameter(names = "-nominatim-import", description = "import nominatim database into photon (this will delete previous index)")
 	private boolean nominatimImport = false;
+
+        @Parameter(names = "-using-languages", description = "languages nominatim importer should import and use at run-time, comma seperated (default: 'en,fr,de,it')")


What about just -languages?

Simple and to the point, much better 👍

yohanboniface · 2014-12-15T16:11:25Z

I think the logic is good :)

I'll let the Java dancers have a deeper look at the details of the implementation :)

Svantulden · 2014-12-16T10:52:14Z

Thanks for reviewing! I reworked some of the code and factorized the adding of collectors (except for the default name adding, because that is a special case unfortunately).

Svantulden · 2014-12-19T09:03:11Z

I tested it to the best of my capabilities, and all seems fine. Do you think this is ready to be merged?

christophlingg · 2014-12-19T09:18:24Z

src/main/java/de/komoot/photon/importer/elasticsearch/Server.java

+                        // get mappings as JSONObject
+                        mappingsString = IOUtils.toString(mappings);
+                } catch(IOException e) {
+			log.error("cannot setup index, elastic search config files not readable", e);


I would rethrow the exception here... otherwise photon would start with a broken mapping and it will break anyway with strange message that make it difficult to understand what's broken. I think it's a good approach to be strict here.

Hmm, wouldn't it be bad practice to rethrow the exception? (see here)

I reworked some of the exception handling regarding the index creating (commit 1a50011), so that the program will not continue and properly throw with the stacktrace. The tests will also fail due to the exception throwing. I would like your opinion on this.

christophlingg · 2014-12-19T09:26:56Z

Hi @Svantulden, I added some comments, we are close to merge it into master!

Svantulden · 2014-12-19T13:28:46Z

Thanks @christophlingg for your feedback!

[Not finished, discussion] Making languages dynamic with command arguments

Sander added 9 commits December 8, 2014 09:55

Fix invalid json

68ae44f

Seperated language commands for import/running

0621d01

Use -nominatim-import-languages for importing and -supported-languages for runtime languages

Added dutch (nl) to mappings.json

35bcfde

Added stripNonDigits util

e9fda4b

Fixed ImporterTest to use (static) languages

dacb243

Added postcode check for nl postcodes

84a37c2

- Filter doubles out if there are highways with the same name & postcode numbers when language is 'nl' (i.e. the same street has 6532RA and 6532BE, which should only be returned once to prevent clutter in autocomplete)

Fixed tests to use new lang scheme

a7ac5eb

Added Netbeans xml to .gitignore

0636de8

yohanboniface reviewed Dec 12, 2014
View reviewed changes

Sander added 7 commits December 15, 2014 13:50

Reverted string lat/lon back to doubles

b4c5d42

Deleted language specific obj from mappings.json

b7e6ff3

Use mappings template & add lang specific strings

fee73da

Removed spaces

41ee0d1

Unified commandline param to 'using-languages'

d81292a

Modified test with static lang

756aeb2

Reverted NL postcode check

f3332f7

- Should be submitted as separate PR

yohanboniface reviewed Dec 15, 2014
View reviewed changes

Sander added 4 commits December 16, 2014 09:03

Removed commented code

0b7f608

Renamed commandline arg to just '-languages'

e4c6a6e

Refactored mappings.json code

0c82ecb

Some refactoring

d586d52

Deleted spaces

80116a9

christophlingg reviewed Dec 19, 2014
View reviewed changes

svantulden added 3 commits December 19, 2014 14:08

Fixes some consistency issues

8b67963

Reworked exception handling for recreateIndex()

1a50011

Removed unneeded null check

c4726f2

christophlingg added a commit that referenced this pull request Dec 20, 2014

Merge pull request #125 from Svantulden/master

e1ab765

[Not finished, discussion] Making languages dynamic with command arguments

christophlingg merged commit e1ab765 into komoot:master Dec 20, 2014

Svantulden changed the title ~~[Not finished, discussion] Making languages dynamic with command arguments~~ Making languages dynamic with command arguments Jul 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making languages dynamic with command arguments #125

Making languages dynamic with command arguments #125

Svantulden commented Dec 10, 2014

yohanboniface commented Dec 12, 2014

yohanboniface Dec 12, 2014

Svantulden Dec 15, 2014

christophlingg commented Dec 12, 2014

Svantulden commented Dec 15, 2014

yohanboniface commented Dec 15, 2014

Svantulden commented Dec 15, 2014

yohanboniface Dec 15, 2014

Svantulden Dec 16, 2014

yohanboniface commented Dec 15, 2014

Svantulden commented Dec 16, 2014

Svantulden commented Dec 19, 2014

christophlingg Dec 19, 2014

Svantulden Dec 19, 2014

christophlingg commented Dec 19, 2014

Svantulden commented Dec 19, 2014

Making languages dynamic with command arguments #125

Making languages dynamic with command arguments #125

Conversation

Svantulden commented Dec 10, 2014

To specify languages that should be imported during nominatim-import

To specify languages for run-time

Thoughts?

Things to do:

yohanboniface commented Dec 12, 2014

yohanboniface Dec 12, 2014

Choose a reason for hiding this comment

Svantulden Dec 15, 2014

Choose a reason for hiding this comment

christophlingg commented Dec 12, 2014

Svantulden commented Dec 15, 2014

yohanboniface commented Dec 15, 2014

Svantulden commented Dec 15, 2014

yohanboniface Dec 15, 2014

Choose a reason for hiding this comment

Svantulden Dec 16, 2014

Choose a reason for hiding this comment

yohanboniface commented Dec 15, 2014

Svantulden commented Dec 16, 2014

Svantulden commented Dec 19, 2014

christophlingg Dec 19, 2014

Choose a reason for hiding this comment

Svantulden Dec 19, 2014

Choose a reason for hiding this comment

christophlingg commented Dec 19, 2014

Svantulden commented Dec 19, 2014