Upgrade to omt-tools v6 and migrate SQL #1246
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
I recommend that #1245 be merged before this PR so that we can have a unit test for the update process before touching update code. |
|
Results evaluating commit 1523be6 (merged with base ac577fe as c00e31e). See run details. PostgreSQL DB size in MB: 2793 ⇒ 2856 (2.3% change)
expand for details... |
Member
|
Thank you for working on this! Could you please just not merging into the |
Contributor
Author
|
@TomPohys I attempted this in #1243, however master-tools will not pass CI as long as the master branch still has the older imposm. So I'm not sure how to get around that but this PR at least holds the SQL fix ready to go for when omt-tools gets upated I guess? See comments by @nyurik: #1243 (comment) Perhaps it might be possible to do a micro-release of omt-tools that upgrades imposm and does nothing else? |
Member
|
Oh, sorry I missed the #1243. Let`s wait for openmaptiles/openmaptiles-tools#378 and then we will release new tools. Is it OK for you? |
Contributor
Author
|
That makes sense to me. I just converted this to draft while we wait for that release. Also I am not sure what's going on with the reported tile size inflation but I suspect it has to do with the newer omt-tools. |
nyurik
changed the title
Member
|
@TomPohys I will postpone run-psql until 6.1 - i am not too happy with the ctrl+c and error handling in it. I have released 6.0, so we can proceed with this merge. |
Member
|
Before merge, the tile size have to be solved. |
Member
|
@TomPohys agree, should figure out what's going on there - i.e. what causes this increase. Note that tile size is computed ONLY from the |
Contributor
Author
|
Since we're seeing the increase as low as z1, we can probably rule out the constraint changes in the transportation table, since transportation objects aren't rendered at that low of a zoom. |
Member
|
I suspect missing gzip, but will have to dig into it tomorrow. |
After some digging, it seems we are not comparing apples to apples. During perf test, we start from a preloaded image, but they have different data between 5.3 and 6.0. This change forces the postgres to 5.3 preloaded data to see if the DB size would change. Also note that the new 6.0 postgres contains specific GEOS version 3.9.1, and also downloads natural earth data from another source - `http://naciscdn.org/naturalearth/packages/natural_earth_vector.sqlite.zip` -> `https://naturalearth.s3.amazonaws.com/packages/natural_earth_vector.sqlite.zip`
Member
|
From 1c25e99: After some digging, it seems we are not comparing apples to apples. During perf test, we start from a preloaded image, but they have different data between 5.3 and 6.0. This change forces the postgres to 5.3 preloaded data to see if the DB size would change. Also note that the new 6.0 postgres contains specific GEOS version 3.9.1, and also downloads natural earth data from another source - |
Contributor
Author
|
Great analysis @nyurik! Because this PR also updates the SQL, I'm concerned about inadvertently breaking something on the SQL side with the transition to dual key. Ideally we could upgrade the postgres version without upgrading the imposm version as a separate step in order to re-baseline the tile size. But, maybe I'm overcomplicating things. |
Member
|
Comparison process -- please try this locally to see what results you get:
running perf-testTo run perf test the same as in C: first run this in the old window inside the test-perf openmaptiles.yaml --minzoom 0 --maxzoom 14 \
--bbox 5.4172943,-1.6732196,12.3733400,4.3475256 \
--bbox 9.0900979,46.9688169,9.6717077,47.5258072 \
--bbox -78.7749754,38.7820235,-76.8957735,39.6985009 \
--bbox -0.6124681,51.2268449,0.3996690,51.7873570 \
--record results.jsonOnce finished, copy the NotesI picked My results for the above tile showed that the v6 had a few more rows than the one before. The layers had a few bytes of changes, not significant enough. Here are the rows that were not included in the previous version -- this be due to the change in GEOS version change (better geometry fixing?), or due to imposm now including more data. The 1% smaller housenumber layer might be due to geometry changes being encoded a bit differently by PBF, but hard to tell without diving into binary comparison. Geometry sizes and all other data is identical |
Member
|
Digging deeper: running Checking the |
Contributor
Author
|
When I ran this comparison between the two branches, I got the same size for the |
Member
|
@ZeLonewolf I'm actually getting the same result now... which is really weird because I got different tables in the other run, which means my testing was flawed... I'm still trying to figure out the problem... Something is fishy..... One thing I did notice already -- by default Imposm used to use |
Member
|
Update: I was correct in the first run -- the tables DO change if I am careful to start from clean slate. The testing steps were updated above. Old Imposm creates fewer rows in these tables from exactly the same data, using exactly the same postgreSQL and PostGIS.
|
Member
|
Another surprising update: manual testing showed no significant change in tile size (under 0.1%)... Seems like our CI perf testing has a bug somewhere. @ZeLonewolf and @TomPohys - can you follow my testing steps above, and see if you get similar results please? |
Contributor
Author
|
Looks like zeros across the board in the posted table for tile size change, unless I'm misunderstanding? |
TomPohys
approved these changes
Oct 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes openmaptiles/openmaptiles-tools#363
This PR migrates the SQL in the transportation and transportation_name layer to use the new imposm3 mappings which now map a separate primary key ID.