Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flex version of the osm2pgsql configuration #4978

Merged
merged 3 commits into from
Oct 26, 2024

Conversation

joto
Copy link
Contributor

@joto joto commented Jun 4, 2024

This commit contains one file with an osm2pgsql configuration for the flex output that can be used instead of the old configuration for the pgsql output. It replaces the openstreetmap-carto.style and openstreetmap-carto.lua files.

The configuration is nearly 100% compatible to the old one.

The database layout will be exactly the same with just very little changes. The id columns (osm_id) and geometry columns (way) on all tables will get the NOT NULL flag when using the flex output. These have always been NOT NULL in practice anyway.

The content of the database will be the same with only minor irrelevant differences.

Run like this:
osm2pgsql -O flex --style openstreetmap-carto-flex.lua -d gis ~/path/to/data.osm.pbf

See #4977

@joto
Copy link
Contributor Author

joto commented Jun 16, 2024

I have

I have not touched the stuff in scripts/lua. It mentions old-style multipolygons and the last change is from 2020, so I am not sure how relevant that still is. It will not work with the new config file and making it work would be a larger effort.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have finished a first reading over the code, not tested it yet.

It looks fairly clean and strait away to understand. Some questions and comments inline.

Regarding the stuff in scripts/lua - that was introduced in #2128 but it was never properly documented how style developers are to use it, the readme essentially discourages its use. As indicated in the past i am very much in favor of introducing more automated testing to support style development and detecting unintentional changes in style behavior. But that would primarily be important for the rendering stage and less for the data import (see https://imagico.de/blog/en/systematic-testing-in-map-design/). And i don't really see this realistically being implemented through volunteer work.

For the data import stage testing - with the move to flex backend including customized processing not only of tags but also of geometries, proper testing would need to include that, i.e. it would need to assert match between input OSM data and output database content - and i don't think this is possible from within LUA.

openstreetmap-carto-flex.lua Show resolved Hide resolved
openstreetmap-carto-flex.lua Show resolved Hide resolved
-- ---------------------------------------------------------------------------

-- Needed for use with the Themepark framework
local themepark = ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with leaving this in for now.

My reasoning is that - while as is there is no substantial benefit from using that - we will need to evaluate that based on the things we ultimately want to do with the flex backend (like the relations and boundary processing).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there is a lot of benefit there immediately for that. One of the most often requested features we get is: Can I have one database with both the data for rendering a map and also Nominatim for geocoding in it. This is possible with the flex output, you "just" need both configuations side by side. And the way to do that is using the Themepark framework which will "call" both configurations. The Nominatim config currently is not Themepark-ready, so we are not quite there yet, but with the Themepark support for OSM Carto you can already have the tables for OSM Carto and for the Shortbread scheme for vector tiles side by side in the same database.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, this is not really our concern here. We have the goal of adaptability and ease of use, but for the database this mainly means we try to ensure that the database schema can be customized easily and that other styles can easily co-use our database. Ensuring that sysadmins have it easier to deploy complex setups with different database schemas together is not our goal.

As said: I am fine with keeping the code for both using and not using themepark in for now and defer the actual decision for when we have a better basis for that. I am not fine with deciding to rely on a framework, which - by its own presentation - is in beta test.

@joto
Copy link
Contributor Author

joto commented Jul 7, 2024

I have rebased the PR to master and added a commit removing the outdated Lua test scripts.

Is there anything else I can do to get this merged?

@imagico
Copy link
Collaborator

imagico commented Jul 8, 2024

Thanks.

I plan to look at this in more detail once we have finished work on #4952 and done a release with that and other pending changes - see #4981.

@joto
Copy link
Contributor Author

joto commented Sep 20, 2024

For your information: The osm2pgsql version 2 released yesterday marks the pgsql output as deprecated.

This commit contains one file with an osm2pgsql configuration for the
flex output that can be used instead of the old configuration for the
pgsql output. It replaces the openstreetmap-carto.style and
openstreetmap-carto.lua files.

The configuration is nearly 100% compatible to the old one.

The database layout will be exactly the same with just very little
changes. The id columns (`osm_id`) and geometry columns (`way`) on all
tables will get the NOT NULL flag when using the flex output. These have
always been NOT NULL in practice anyway.

The content of the database will be the same with only minor irrelevant
differences.

Run like this:
osm2pgsql -O flex --style openstreetmap-carto-flex.lua -d gis ~/path/to/data.osm.pbf
This commit does the actual switch to the new flex config. It removes
the old config files and updates the documentation and various scripts.
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the long time it took us to look at this.

On first reading and testing this looks excellent. Comparing the database content with my abstract test data before and after the only differences i can see are:

  • the already mentioned NOT NULL contraints - which are actually a plus i think - and which should not affect backwards compatibility.
  • minor differences in way_area values of the polygons (natural, since the calculation changed internally).
  • some differences in order of rows in the dump - not sure about the reason for those, but practically irrelevant.

It would be nice if someone did test this working on the docker setup - but that is essentially unmaintained - see #5005. Still the file name should be fixed (the only request for changes i have).

We currently don't specify a minimum version of osm2pgsql required in INSTALL.md - @joto: if you know the minimum version necessary for running this we could add that (and could also update the link from https://github.com/openstreetmap/osm2pgsql#installing to https://github.com/osm2pgsql-dev/osm2pgsql#installing).

I would like to encourage all active OSM-Carto developers to test this and report any problems.

--style openstreetmap-carto.style \
--tag-transform-script openstreetmap-carto.lua \
--output flex \
--style openstreetmap-carto-flex.style \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested the docker scripts (nor am i going to) - but this is the wrong file name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

They don't work any more with the new flex Lua output. And they are
have not been maintained anyway.
@joto
Copy link
Contributor Author

joto commented Oct 19, 2024

I have fixed the file name in the docker script and added the minimum osm2pgsql version (1.8.0) to the INSTALL file (and fixed some other things there pertaining to the change).

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks fine to me.

@pnorman - since you worked previously on this matter your input would be helpful. It is my understanding that each of your further going changes in #4431 could still be implemented on top of this if we decide to do so. We will undoubtedly want to add the relations to the database and we will need to look at the administrative boundaries as well (see comments in #4431 (review)).

My thought is (and i have indicated so in the past) that going step by step and having a fully backwards compatible version with the flex backend first is going to be helpful, in particular for derivative styles. And our developers and designers can better familiarize themselves with the new techniques step by step this way.

@mboeringa
Copy link

mboeringa commented Oct 20, 2024

@pnorman - since you worked previously on this matter your input would be helpful. It is my understanding that each of your further going changes in #4431 could still be implemented on top of this if we decide to do so. We will undoubtedly want to add the relations to the database and we will need to look at the administrative boundaries as well (see comments in #4431 (review)).

@imagico

I have extensively used, and adjusted, a derivative of Paul's version of an openstreetmap-carto Lua flex style, and do indeed think any adjustments could be applied later. As to the specific "boundary line" issue, I do think that switching to another type of boundary styling is prudent also.

The whole point of switching to line based boundary styling without duplicate overlapping polygon outlines, is that you can switch to more flexible types of styling, e.g. something like open hatch cross type you see in for example Swiss Topo maps. With styling based on polygon outlines, you need to "hide" lower level admin levels by overlapping them in the right order and use full fill line styling. This is not necessary with de-duplicated boundary line styling, you get more flexibility in cartographic styling for the lines.

My thought is (and i have indicated so in the past) that going step by step and having a fully backwards compatible version with the flex backend first is going to be helpful, in particular for derivative styles. And our developers and designers can better familiarize themselves with the new techniques step by step this way.

My personal derivative style is adjustable by changing settings in a top configuration setting that I created, in combination with extensive usage of if..then..else constructs to change the osm2pgsql process flow. This allows you to either create a "backwards compatible" database schema, or a significantly adjusted modernized schema. I realize this may not be a desirable scenario for the "official" Lua openstreetmap-carto style due to added complexity in code (although the configuration actually makes direct usage and basic adjustments easier), but it does show the kind of back- and forward compatibility you can achieve.

@imagico
Copy link
Collaborator

imagico commented Oct 20, 2024

Please no mixing of this with more specific considerations of how to render administrative boundaries. These should either go to #2172 (if it is about low zoom level rendering) or into a new issue.

I think i need to explain the concept of backwards compatibility more clearly - what i am talking about with that in this context is that the newer and older versions of this style continue being usable from the same rendering database.

  • full backwards compatibility (which this change currently maintains) means the database produced after the change can be used directly to render older versions of the style (or forks that use an older version of the database setup) without modifications. We have had this mostly since we moved to hstore IIRC.
  • limited backwards compatibility means the new layout can be used to render older versions with just minor modifications (like adjusting references to use columns vs. hstore) or relatively simple database views. We have maintained this since the beginning of the project. You can render OSM-Carto v1.0.0 with the current database schema with just a few minor adjustments in the code.

Backwards compatibility in that sense does not prevent us from introducing new database structures in support of our map design goals. But we should also not be afraid of breaking with backwards compatibility if it is of clear benefit for us (see comment here) - we should just be mindful of this effect and make a conscious decision.

The question if several different database schemas can be generated (or used) with the same codebase - and the means to be able to do so with limited code complexity and low maintenance effort - is a different matter. This is currently something that is not part of our goals.

@mboeringa
Copy link

  • full backwards compatibility (which this change currently maintains) means the database produced after the change can be used directly to render older versions of the style (or forks that use an older version of the database setup) without modifications. We have had this mostly since we moved to hstore IIRC.

@imagico. I agree maximizing backwards compatibility, only requiring switching to 'flex' and changing the osm2pgsql command line for import & updates accordingly, is desirable for now. This will have minimal impact for downstream users, unless they already work with their own heavily modified osm2pgsql 'pgsql' Lua openstreetmap-carto derivative style, but in that case they are likely already investigating 'flex' to do the transition themselves, or have already done so.

@imagico imagico merged commit d9ea31c into gravitystorm:master Oct 26, 2024
2 checks passed
@imagico
Copy link
Collaborator

imagico commented Oct 26, 2024

I have merged this now - thanks for the patience.

I have also opened #5027 for the next steps following from this change - towards a new major release, towards integrating previous work of @pnorman on #4431 and towards an overall strategy in using the flex backend - in line with our goals and with previous discussion on various issues. Everyone is welcome to contribute there or to open new issues on the specific todos listed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants