Enhance valhalla_build_elevation with LZ4 recompression support #3607
Conversation
| elif config is not None: | ||
| elevation_fp = Path(config["additional_data"]["elevation"] or "elevation") |
There was a problem hiding this comment.
this is not quite enough to make config optional as it's also used further down when the user wants to get tiles covering the graph (often the most sensible option IMO in case elevation is only used to enrich the graph, not the /height service). admittedly there was no exception handling in place before either, config wasn't even required.
guess it's enough to protect line 315 from missing config path.
There was a problem hiding this comment.
I guess I should point out the motivation for touching this as well. While the normal case would have a Valhalla JSON config, I think it's generally useful to have an option to run this script without that context, such as to download the whole planet elevation data to an internal file server. This is probably a minority use case, but I think it's useful to be able to operate as the previous shell script without a JSON config.
That said, good call. I neglected the possible code path on line 315. Any objection to checking that config is not none, exiting if it's not, and updating docs? This might be possible in argparse, but I don't know a great way off the top of my head.
There was a problem hiding this comment.
yeah I figured so, should've commented that I thought it's a valid use case, now the script can be used independently of valhalla. just get it from github's raw endpoint and execute it to get some bbox's tilezen tiles, that's great. would you mind adding some notes about that to the PR description for future reference?
Any objection to checking that config is not none, exiting if it's not, and updating docs
that's what I had in mind too:)
There was a problem hiding this comment.
Sure thing; I'll update the PR description.
Co-authored-by: Nils <[email protected]>
Issue
Closes #3606.
Tasklist
Requirements / Relations
Complementary to #3401.
Commentary
The main goal of this PR is to add support for re-compressing downloaded elevation tiles as LZ4. LZ4 comes at a slight space cost (roughly 12% larger than gzip for the planet) and of course the upfront CPU cost of recompressing. In real-world request loads, we have observed significant reductions in p95 and higher response times after switching from gzip to LZ4 elevation tiles.
Secondarily, this PR makes a subtle type change from a boolean decompress flag to explicitly supporting multiple compression types in case something better comes along later. This also ensures the added cases are cleanly handled. It also removes the requirement for a JSON config file. While a JSON config file is probably the more typical usage pattern, it's also useful to be able to specify a bbox and output directory on the command line directly when downloading the whole world to a local file server or similar applications (and it technically doesn't even have to be Valhalla-specific).