mediagis / nominatim-docker

100% working container for Nominatim
Creative Commons Zero v1.0 Universal
1.07k stars 437 forks source link

Support nominatim 4.3.0 #476

Closed iAlex97 closed 11 months ago

iAlex97 commented 11 months ago

As Nominatim 4.3.0 is out for about 2 weeks, this PR adds necessary changes to build and run the new version.

Major differences from previous version deduced from Installation Prerequisites:

  1. Dependencies:
    • Build time:
    • nlohmann/json
    • Run-time:
    • SQLAlchemy
    • asyncpg
      1. An issue originating from asyncpg resulting in PermissionError: [Errno 13] Permission denied: '/root/.postgresql/postgresql.key' after import (for more details see this)
what-the-diff[bot] commented 11 months ago

PR Summary

iAlex97 commented 11 months ago

Thank you for such a quick review @philipkozeny, I love this project and I'm glad I can contribute to it.

I just did the modifications along with removing a forgotten environment print. Can we stop the older pipelines from running?

leonardehrenfried commented 11 months ago

I've cancelled them just now. Not sure if you need admin privileges for that./

iAlex97 commented 11 months ago

I'm not really familiar with GitHub workflows, but I checked the "Actions" tab and I only had "view" related options for running pipelines, so it's most likely you need admin privileges 🤷

leonardehrenfried commented 11 months ago

Do you know what this failure is about?

+ sudo -H -E -u nominatim nominatim admin --warm --reverse
2023-09-28 12:10:38: Using project directory: /nominatim
2023-09-28 12:10:38: Warming database caches
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/sqlalchemy/dialects/postgresql/asyncpg.py", line 399, in _prepare_and_execute
    prepared_stmt, attributes = await adapt_connection._prepare(
  File "/usr/lib/python3/dist-packages/sqlalchemy/dialects/postgresql/asyncpg.py", line 641, in _prepare
    prepared_stmt = await self._connection.prepare(operation)
  File "/usr/lib/python3/dist-packages/asyncpg/connection.py", line 566, in prepare
    return await self._prepare(
  File "/usr/lib/python3/dist-packages/asyncpg/connection.py", line 584, in _prepare
    stmt = await self._get_statement(
  File "/usr/lib/python3/dist-packages/asyncpg/connection.py", line 398, in _get_statement
    statement = await self._protocol.prepare(
  File "asyncpg/protocol/protocol.pyx", line 168, in prepare
asyncpg.exceptions.UndefinedTableError: relation "search_name" does not exist
leonardehrenfried commented 11 months ago

Is this a timing issue or is nominatim's --warm --reverse option broken in 4.3?

iAlex97 commented 11 months ago

I'm not sure at this point, will check locally to see if I can reproduce it

iAlex97 commented 11 months ago

It seems that I get this issue locally too, so it's not a timing problem.

Checking the changelog for 4.3.0, my wild guess would be that this has something to do with "reorganise osm2pgsql flex style and make it the default"

I have several questions regarding this:

leonardehrenfried commented 11 months ago

We can try tagging @lonvia. Maybe she has an idea if nominatim admin --warm --reverse is supposed to work.

mtmail commented 11 months ago

Now tracked at https://github.com/osm-search/Nominatim/issues/3213

mtmail commented 11 months ago

I think the parameter names changed slightly. nominatim admin --warm --search-only should work.

iAlex97 commented 11 months ago

Thank you @mtmail, reverse-only imports are no longer failing using that parameter.

iAlex97 commented 11 months ago

However, I now have another dilemma: should we switch back to using --reverse after https://github.com/osm-search/Nominatim/issues/3213 is released with a new version of 4.3? My thinking is that in case the two flags are equivalent, --reverse should be deprecated and removed, otherwise we should still be using --reverse after it is fixed.

Edit: never mind, I read your comment now @mtmail :)

lonvia commented 11 months ago

That's bad enough of a regression that I'm inclined to do a patch release of Nominatim. Won't happen before next week, though.

iAlex97 commented 11 months ago

I have added comments before the warmup command, you guys let me know if you want to merge this as is or wait for the patch release. I haven't committed them yet because I don't want to run the pipelines for basically no changes 😅

leonardehrenfried commented 11 months ago

I would be in favour of having a release soonish and then fix it whenever 4.3.1 is released.

iAlex97 commented 11 months ago

All right, I would also like the release to happen sooner rather than later to be able to start a full planet import on some servers. I will commit the comments and we can stop the CI pipeline as the last on completed without error.

Last thing I would like to address is PermissionError: [Errno 13] Permission denied: '/root/.postgresql/postgresql.key'. After setting the -H flag for sudo -H -E -u nominatim nominatim admin --warm which changes the HOME env variable from the parent shells' to the users' home, I would have expected /home/nominatim/.postgresql/postgresql.key to exist but not even the .postgresql directory is created.

Do you think that this file is critical? I tried searching for its purpose online, but couldn't find anything relevant.

philipkozeny commented 11 months ago

All right, I would also like the release to happen sooner rather than later to be able to start a full planet import on some servers. I will commit the comments and we can stop the CI pipeline as the last on completed without error.

Last thing I would like to address is PermissionError: [Errno 13] Permission denied: '/root/.postgresql/postgresql.key'. After setting the -H flag for sudo -H -E -u nominatim nominatim admin --warm which changes the HOME env variable from the parent shells' to the users' home, I would have expected /home/nominatim/.postgresql/postgresql.key to exist but not even the .postgresql directory is created.

Do you think that this file is critical? I tried searching for its purpose online, but couldn't find anything relevant.

Where do you see that error? Searching through the logs of the CI checks I can't find those.

iAlex97 commented 11 months ago

I encountered that error while trying to get the new version up the first time. It seems like asyncpg tries to write this file in the user's home directory, however when doing sudo -E -u nominatim the HOME environment variable was still set to /root. I got around this issue by using the -H parameter which would set it to /home/nominatim so that user nominatim is able to write to it.

My import was working as I did a few queries, but I don't know If it has performance implications or whatnot.

Full stacktrace from local logs:

+ sudo -E -u nominatim nominatim admin --warm
2023-09-28 10:01:12: Using project directory: /nominatim
2023-09-28 10:01:12: Warming database caches
Traceback (most recent call last):
  File "/usr/local/bin/nominatim", line 12, in <module>
    exit(cli.nominatim(module_dir='/usr/local/lib/nominatim/module',
  File "/usr/local/lib/nominatim/lib-python/nominatim/cli.py", line 225, in nominatim
    return get_set_parser().run(**kwargs)
  File "/usr/local/lib/nominatim/lib-python/nominatim/cli.py", line 121, in run
    return args.command.run(args)
  File "/usr/local/lib/nominatim/lib-python/nominatim/clicmd/admin.py", line 60, in run
    return self._warm(args)
  File "/usr/local/lib/nominatim/lib-python/nominatim/clicmd/admin.py", line 95, in _warm
    api.reverse((random.uniform(-90, 90), random.uniform(-180, 180)),
  File "/usr/local/lib/nominatim/lib-python/nominatim/api/core.py", line 610, in reverse
    return self._loop.run_until_complete(self._async_api.reverse(coord, **params))
  File "/usr/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/usr/local/lib/nominatim/lib-python/nominatim/api/core.py", line 201, in reverse
    async with self.begin() as conn:
  File "/usr/lib/python3.10/contextlib.py", line 199, in __aenter__
    return await anext(self.gen)
  File "/usr/local/lib/nominatim/lib-python/nominatim/api/core.py", line 140, in begin
    await self.setup_database()
  File "/usr/local/lib/nominatim/lib-python/nominatim/api/core.py", line 101, in setup_database
    async with engine.begin() as conn:
  File "/usr/lib/python3/dist-packages/sqlalchemy/ext/asyncio/base.py", line 60, in __aenter__
    return await self.start(is_ctxmanager=True)
  File "/usr/lib/python3/dist-packages/sqlalchemy/ext/asyncio/engine.py", line 609, in start
    await self.conn.start(is_ctxmanager=is_ctxmanager)
  File "/usr/lib/python3/dist-packages/sqlalchemy/ext/asyncio/engine.py", line 154, in start
    await (greenlet_spawn(self.sync_engine.connect))
  File "/usr/lib/python3/dist-packages/sqlalchemy/util/_concurrency_py3k.py", line 134, in greenlet_spawn
    result = context.throw(*sys.exc_info())
  File "/usr/lib/python3/dist-packages/sqlalchemy/future/engine.py", line 406, in connect
    return super(Engine, self).connect()
  File "/usr/lib/python3/dist-packages/sqlalchemy/engine/base.py", line 3204, in connect
    return self._connection_cls(self, close_with_result=close_with_result)
  File "/usr/lib/python3/dist-packages/sqlalchemy/engine/base.py", line 96, in __init__
    else engine.raw_connection()
  File "/usr/lib/python3/dist-packages/sqlalchemy/engine/base.py", line 3283, in raw_connection
    return self._wrap_pool_connect(self.pool.connect, _connection)
  File "/usr/lib/python3/dist-packages/sqlalchemy/engine/base.py", line 3250, in _wrap_pool_connect
    return fn()
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/base.py", line 310, in connect
    return _ConnectionFairy._checkout(self)
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/base.py", line 868, in _checkout
    fairy = _ConnectionRecord.checkout(pool)
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/base.py", line 476, in checkout
    rec = pool._do_get()
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/impl.py", line 145, in _do_get
    with util.safe_reraise():
  File "/usr/lib/python3/dist-packages/sqlalchemy/util/langhelpers.py", line 70, in __exit__
    compat.raise_(
  File "/usr/lib/python3/dist-packages/sqlalchemy/util/compat.py", line 207, in raise_
    raise exception
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/impl.py", line 143, in _do_get
    return self._create_connection()
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/base.py", line 256, in _create_connection
    return _ConnectionRecord(self)
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/base.py", line 371, in __init__
    self.__connect()
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/base.py", line 665, in __connect
    with util.safe_reraise():
  File "/usr/lib/python3/dist-packages/sqlalchemy/util/langhelpers.py", line 70, in __exit__
    compat.raise_(
  File "/usr/lib/python3/dist-packages/sqlalchemy/util/compat.py", line 207, in raise_
    raise exception
  File "/usr/lib/python3/dist-packages/sqlalchemy/pool/base.py", line 661, in __connect
    self.dbapi_connection = connection = pool._invoke_creator(self)
  File "/usr/lib/python3/dist-packages/sqlalchemy/engine/create.py", line 590, in connect
    return dialect.connect(*cargs, **cparams)
  File "/usr/lib/python3/dist-packages/sqlalchemy/engine/default.py", line 597, in connect
    return self.dbapi.connect(*cargs, **cparams)
  File "/usr/lib/python3/dist-packages/sqlalchemy/dialects/postgresql/asyncpg.py", line 758, in connect
    await_only(self.asyncpg.connect(*arg, **kw)),
  File "/usr/lib/python3/dist-packages/sqlalchemy/util/_concurrency_py3k.py", line 76, in await_only
    return current.driver.switch(awaitable)
  File "/usr/lib/python3/dist-packages/sqlalchemy/util/_concurrency_py3k.py", line 129, in greenlet_spawn
    value = await result
  File "/usr/lib/python3/dist-packages/asyncpg/connection.py", line 2085, in connect
    return await connect_utils._connect(
  File "/usr/lib/python3/dist-packages/asyncpg/connect_utils.py", line 874, in _connect
    addrs, params, config = _parse_connect_arguments(timeout=timeout, **kwargs)
  File "/usr/lib/python3/dist-packages/asyncpg/connect_utils.py", line 640, in _parse_connect_arguments
    addrs, params = _parse_connect_dsn_and_args(
  File "/usr/lib/python3/dist-packages/asyncpg/connect_utils.py", line 543, in _parse_connect_dsn_and_args
    if not sslkey.exists():
  File "/usr/lib/python3.10/pathlib.py", line 1290, in exists
    self.stat()
  File "/usr/lib/python3.10/pathlib.py", line 1097, in stat
    return self._accessor.stat(self, follow_symlinks=follow_symlinks)
PermissionError: [Errno 13] Permission denied: '/root/.postgresql/postgresql.key'
philipkozeny commented 11 months ago

@iAlex97 I can't reproduce it locally and I don't see the error in the test pipeline, so I think we can move forward and merge it. @leonardehrenfried do you agree?

leonardehrenfried commented 11 months ago

I'm happy ~to merge~ to have this merged

iAlex97 commented 11 months ago

Cool @leonardehrenfried, as I don't have write permissions I'd need someone to merge this PR for me 😅