Modify

Opened 5 years ago

Closed 5 months ago

#12303 closed enhancement (fixed)

[Patch] When downloading objects with Overpass API, use recurse up to fetch referrers

Reported by: simon04 Owned by: simon04
Priority: normal Milestone: 20.06
Component: Core Version:
Keywords: mirrored_download overpass Cc: Penegal, tyr_asd

Description

Since #7670, "Download object" can use the Overpass API instead of the OSM API. Since the Overpass API supports the recurse up operator <;, we could make use of the for fetching referrers of the object.

Launching "Download object" with n288207873 and "Download referrers" as well as "Download relation members" set, we have a bunch of HTTP requests:

INFO: GET https://api.openstreetmap.org/api/0.6/nodes?nodes=288207873 -> 200
INFO: GET https://api.openstreetmap.org/api/0.6/node/288207873/ways -> 200 (943 B)
INFO: Way 26,309,072 with 85 nodes has incomplete nodes because at least one node was missing in the loaded data.
INFO: Way 25,648,174 with 13 nodes has incomplete nodes because at least one node was missing in the loaded data.
INFO: GET https://api.openstreetmap.org/api/0.6/node/288207873/relations -> 200 (184 B)
INFO: GET https://api.openstreetmap.org/api/0.6/way/26309072/full -> 200
INFO: GET https://api.openstreetmap.org/api/0.6/way/25648174/full -> 200
INFO: GET https://api.openstreetmap.org/api/0.6/nodes?nodes=288207917,288207914,1088131027,288207911,2939204923,288207910,278027141,288207924,288207920,1088131320,1088131071,288207882,2217273857,2217273859,1088131312,288207872,1318297762,1088131176,391657967,1318297764,1088130924,1088131040,288207853,2217262568,1088131099,288207852,2217273833,279583979,288207855,288207854,2217273835,2217273837,288207851,2217273839,288207845,288207844,279583971,288207847,288207846,2217062382,288977653,288207841,288207840,2217262566,288207843,288977655,288207842,2217273831,2217062378,415506827,288207869,288207868,288207871,288207870,288207865,288207864,372888428,530245199,288207867,2217273854,372888431,288207866,288207861,2217273840,279583986,288207860,288207863,288207862,2217273843,288207857,288207856,2217273845,288207859,288207858,2217273847,2217262538,1088131007,279583950,279583951,2217262531,2217262533,1088131254,2217262534,288207939,1088130986,288207839,2217262556,279583964,2217262544,2217262547,2217262548,2217262549,279583959,2217262551,1088130980 -> 200

Ideally, we could strip this down to one Overpass call. The question is just how to combine node(288207873) and >; and <; in order to accomplish this. Help is welcome … :)

Attachments (10)

12303.patch (8.6 KB) - added by GerdP 10 months ago.
12303.2.patch (14.7 KB) - added by GerdP 10 months ago.
work in progress
12303.3.patch (14.5 KB) - added by GerdP 10 months ago.
12303.4.patch (19.4 KB) - added by GerdP 10 months ago.
12303.5.patch (19.5 KB) - added by GerdP 6 months ago.
updated patch, doesn't do a full recursive download of children
missing-1212.PNG (10.8 KB) - added by GerdP 6 months ago.
deleted-1212.PNG (6.4 KB) - added by GerdP 6 months ago.
12303.6.patch (31.9 KB) - added by GerdP 6 months ago.
missing-1212-overpas.PNG (10.4 KB) - added by GerdP 6 months ago.
12303.7.patch (32.0 KB) - added by GerdP 6 months ago.
tr -> trn

Download all attachments as: .zip

Change History (40)

comment:1 Changed 5 years ago by simon04

Milestone: 16.0216.03

comment:2 Changed 5 years ago by Don-vip

Milestone: 16.0316.04

Milestone renamed

comment:3 Changed 5 years ago by Don-vip

Milestone: 16.0416.05

comment:4 Changed 4 years ago by Don-vip

Milestone: 16.0516.06

comment:5 Changed 4 years ago by Don-vip

Keywords: overpass added
Milestone: 16.0616.07

comment:6 Changed 4 years ago by Don-vip

Milestone: 16.0716.08

comment:7 Changed 4 years ago by Don-vip

Milestone: 16.08

comment:8 Changed 3 years ago by anonymous

This probably can in fact now be reduced to one Overpass call using the 'complete' block statement, new to Overpass API 0.7.55, which does the recursion for referrers on the server side. Using the right input set and most probably a loop limit of 1 (simply enough to get parents of the newly discovered elements that aren't part of the original union query)...

https://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL#The_block_statement_complete

comment:9 Changed 2 years ago by simon04

Ticket #16537 has been marked as a duplicate of this ticket.

comment:10 Changed 2 years ago by simon04

Cc: Penegal tyr_asd added

comment:11 Changed 10 months ago by GerdP

Owner: changed from simon04 to GerdP
Status: newassigned

See also #18566 and #18624. Working on a class which should calculate the best strategy to download the wanted objects, using either Overpass API or OSM multi-fetch or the /full suffix.

comment:12 Changed 10 months ago by GerdP

The corresponding single overpass query for a list of objects could look like this

(
  node(id:70674932,...); 
  way(id:29201179,...) -> .w; 
  rel(id:69202,...) -> .r;
) -> .i;  // .i : the input set
(
  .i;rel(br); // if any relations are in the input list
  .i;rel(bw); // if any ways ...
  .i;rel(bn);(.i;way(bn);>;); // if any nodes ...
) -> .p; // .p contains the parents and the input set
(.p; - (.w;.r;);); // subtract relations and ways that were in the input set
out meta;

The actual query depends on the existence of the elements. If the initial list doesn't contain relations some parts of the query would be omitted.
The result of this query should be the same as the sequnce of API requests done by the current code.

comment:13 Changed 10 months ago by GerdP

Ah, sorry, forget the above. The subtract clause doesn't work. This was a last minute change. I try to solve the problem that I sometimes get more data then wanted.

comment:14 Changed 10 months ago by GerdP

removed...

Last edited 10 months ago by GerdP (previous) (diff)

Changed 10 months ago by GerdP

Attachment: 12303.patch added

comment:15 Changed 10 months ago by GerdP

Owner: changed from GerdP to simon04
Status: assignedneedinfo
Summary: When downloading objects with Overpass API, use recurse up to fetch referrers[Patch] When downloading objects with Overpass API, use recurse up to fetch referrers

The patch implements the retrival of parents via overpass api if that overpass api is enabled. This is faster when many objects are requested but typically much slower for a single object.
Not sure if I should change the code so that it doesn't use overpass for less than - say - 5 objects?

comment:16 Changed 10 months ago by skyper

At least, leave overpass as fallback if OSM-Server is not responding/down.

comment:17 Changed 10 months ago by GerdP

Do you mean to use overpass if OSM server produced a timeout while requesting parents for a small number of objects?

comment:18 in reply to:  17 Changed 10 months ago by skyper

Replying to GerdP:

Do you mean to use overpass if OSM server produced a timeout while requesting parents for a small number of objects?

Yes, if you set it to only use OSM server for a small number.

comment:19 Changed 10 months ago by simon04

By default, download.overpass.for-multi-fetch is false and thus the new way of fetching is inactive.

For the example from the ticket description, still two requests are made:

2020-02-11 21:27:12.479 INFO: POST https://overpass-api.de/api/interpreter (25 B) ...
2020-02-11 21:27:12.479 TRACE: BODY: node(288207873);out meta;
2020-02-11 21:27:13.614 INFO: POST https://overpass-api.de/api/interpreter -> HTTP/1.1 200 (976 ms; 362 B)
2020-02-11 21:27:13.619 INFO: POST https://overpass-api.de/api/interpreter (85 B) ...
2020-02-11 21:27:13.619 TRACE: BODY: (node(288207873)->.n;.n;rel(bn);.n;way(bn);)->.pn;((.pn;);node(w);node(r););out meta;
2020-02-11 21:27:14.950 INFO: POST https://overpass-api.de/api/interpreter -> HTTP/1.1 200 (1.2 s; 2.93 kB)

Concerning the implementation, you could simplify genOverpassQuery by using some Java 8 features:

        TreeMap<OsmPrimitiveType, Set<PrimitiveId>> primitivesMap = children.stream()
                .collect(Collectors.groupingBy(PrimitiveId::getType, TreeMap::new, Collectors.toSet()));

The switch cases CLOSEDWAY and MULTIPOLYGON are irrelevant.

comment:20 Changed 10 months ago by GerdP

Yes, there is still a lot to improve. I think we have too many different classes for the download tasks.

reg. Java 8 features: I think many of them are almost unreadable, and this is one of them. I only understand what it does because I know what it should replace.

reg. switch: I think I added them only to calm down SonarLint. I know they are not needed.

comment:21 in reply to:  20 Changed 10 months ago by simon04

Replying to GerdP:

reg. Java 8 features: I think many of them are almost unreadable, and this is one of them.

For me it's the opposite: once getting used to the effect of the various terminal operation of streams, expressions involving streams are much more concise (to read) and better convey the programmer's intent. I think the expression Collectors.groupingBy(PrimitiveId::getType) is a good example for my point.

comment:22 in reply to:  19 Changed 10 months ago by GerdP

Replying to simon04:

By default, download.overpass.for-multi-fetch is false and thus the new way of fetching is inactive.

For the example from the ticket description, still two requests are made:

2020-02-11 21:27:12.479 INFO: POST https://overpass-api.de/api/interpreter (25 B) ...
2020-02-11 21:27:12.479 TRACE: BODY: node(288207873);out meta;
2020-02-11 21:27:13.614 INFO: POST https://overpass-api.de/api/interpreter -> HTTP/1.1 200 (976 ms; 362 B)
2020-02-11 21:27:13.619 INFO: POST https://overpass-api.de/api/interpreter (85 B) ...
2020-02-11 21:27:13.619 TRACE: BODY: (node(288207873)->.n;.n;rel(bn);.n;way(bn);)->.pn;((.pn;);node(w);node(r););out meta;
2020-02-11 21:27:14.950 INFO: POST https://overpass-api.de/api/interpreter -> HTTP/1.1 200 (1.2 s; 2.93 kB)

As a next step I'll move all code to generate overpass queries to static methods in MultiFetchOverpassObjectReader.

Changed 10 months ago by GerdP

Attachment: 12303.2.patch added

work in progress

comment:23 Changed 10 months ago by GerdP

The patch implements the static method, but I don't see yet where to use it.
For the example above the generated query looks like this:

node(288207873)->.n;.n;way(bn)->.wn;.n;rel(bn)->.rn;((.n;.wn;.rn;);node(w););out meta;

Performance isn't great. There are so many ways to get the wanted result and I am not yet sure which one performs best...

Changed 10 months ago by GerdP

Attachment: 12303.3.patch added

comment:24 Changed 10 months ago by GerdP

12303.3.patch generates this which seems faster:

node(288207873)->.n;.n;way(bn)->.wn;.n;rel(bn)->.rn;(.wn;node(w);.rn;);out meta;

The possible query is only logged for now, see comment:23

Changed 10 months ago by GerdP

Attachment: 12303.4.patch added

comment:25 Changed 10 months ago by GerdP

With the new patch a single download is performed when overpass api is enabled.

@Simon04: Please let me know what you think about the new class DownloadFromOverpassTask. I'd rather prefer to reduce the number of DownloadXYZTask classes but I don't fully understand the threading concept.

There is more to do to handle the fact the overpass doesn't return deleted objects.
Try to download deleted node 1212 with the different APIs.

comment:26 Changed 6 months ago by simon04

Milestone: 20.06

comment:27 Changed 6 months ago by GerdP

The patch was written before I reverted the full recursive download of child relations for the normal API. See #18835
So, this needs more work.

Changed 6 months ago by GerdP

Attachment: 12303.5.patch added

updated patch, doesn't do a full recursive download of children

Changed 6 months ago by GerdP

Attachment: missing-1212.PNG added

Changed 6 months ago by GerdP

Attachment: deleted-1212.PNG added

comment:28 Changed 6 months ago by GerdP

Up to now the overpass api doesn't allow to retrieve deleted objects. We simply get no data. Current code first showss this
(wrong) message when you try to download node 1212 from overpass:

sometimes followed by another message

I've not yet found out why the 2nd message appears only sometimes.
The big problem is that we cannot distinguish between a node that was deleted and one which never existed. If you try to download a node 1234567891234 you get a similar response from overpass.

Changed 6 months ago by GerdP

Attachment: 12303.6.patch added

Changed 6 months ago by GerdP

Attachment: missing-1212-overpas.PNG added

Changed 6 months ago by GerdP

Attachment: 12303.7.patch added

tr -> trn

comment:29 Changed 6 months ago by GerdP

I've not yet found out why the 2nd message appears only sometimes.

My bad. Happened only when the layer already contained the data about the deleted node from a previous download via OSM API. The new patch handles this better.
New popup message looks like this when overpass is used:

New i18n strings for this:

trn("The server did not return data for the requested object, it was either deleted or does not exist.",
"The server did not return data for the requested objects, they were either deleted or do not exist.",
errs.size());

If both parents and members are downloaded the generated overpass query can be very complex even if only one object is downloaded.

relation(2732622)->.r;.r;rel(br)->.pr;.r;rel(r)->.rm;(.r;.pr;.r;>;.rm;);out meta;

Maybe one of the overpass devs could review them?

comment:30 Changed 5 months ago by GerdP

Resolution: fixed
Status: needinfoclosed

In 16611/josm:

fix #12303: When downloading objects with Overpass API, use recurse up to fetch referrers

  • new static method genOverpassQuery() to generate a single overpass query for all wanted objects
  • use POST instead of PUT to send the query
  • add handling for missing primitives (Overpass doesn't return invisible objects and doesn't a rc 404)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.