-
Notifications
You must be signed in to change notification settings - Fork 448
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
Support nominatim 4.3.0 #476
Conversation
PR Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iAlex97 First off, thank you for taking the time to contribute to this project! 🎉
I've reviewed your PR, I just have a small request in the docker-compose contrib files. I genuinely appreciate the effort and dedication you've put into this.
nominatim: | ||
container_name: nominatim | ||
image: mediagis/nominatim:4.3 | ||
restart: always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the restart policy, since we had problems in the past with people not checking their import process when it fails? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thing, I removed it now
4.3/contrib/docker-compose.yml
Outdated
nominatim: | ||
container_name: nominatim | ||
image: mediagis/nominatim:4.3 | ||
restart: always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
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? |
I've cancelled them just now. Not sure if you need admin privileges for that./ |
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 🤷 |
Do you know what this failure is about?
|
Is this a timing issue or is nominatim's |
I'm not sure at this point, will check locally to see if I can reproduce it |
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:
|
We can try tagging @lonvia. Maybe she has an idea if |
Now tracked at osm-search/Nominatim#3213 |
I think the parameter names changed slightly. |
Thank you @mtmail, reverse-only imports are no longer failing using that parameter. |
However, I now have another dilemma: should we switch back to using Edit: never mind, I read your comment now @mtmail :) |
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. |
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 😅 |
I would be in favour of having a release soonish and then fix it whenever 4.3.1 is released. |
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 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. |
I encountered that error while trying to get the new version up the first time. It seems like 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:
|
@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? |
I'm happy |
Cool @leonardehrenfried, as I don't have write permissions I'd need someone to merge this PR for me 😅 |
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:
asyncpg
resulting inPermissionError: [Errno 13] Permission denied: '/root/.postgresql/postgresql.key'
after import (for more details see this)