-
Notifications
You must be signed in to change notification settings - Fork 0
add GHA skeleton for routing regression pipeline #12
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
base: main
Are you sure you want to change the base?
Changes from all commits
4bfc820
9cbc9b8
383db98
84867be
8635d36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| name: Build Tiles | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| valhalla_ref: | ||
| description: "Valhalla ref to use for tile building (branch, tag, or SHA)" | ||
| default: master | ||
| required: false | ||
|
|
||
| jobs: | ||
| build-valhalla: | ||
| uses: ./.github/workflows/build-valhalla.yml | ||
| with: | ||
| valhalla_ref: ${{ inputs.valhalla_ref }} | ||
| secrets: inherit | ||
|
Comment on lines
+12
to
+16
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems like a redundant job..
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. build-valhalla now produces a wheel artifact that build-tiles downloads and installs, no rebuilding inline |
||
|
|
||
| build-tiles: | ||
| runs-on: ubuntu-latest | ||
| needs: build-valhalla | ||
|
|
||
| steps: | ||
| - name: Sanitize ref for artifact name | ||
| id: sanitize | ||
| run: echo "ref_slug=$(echo '${{ inputs.valhalla_ref }}' | tr '/' '-')" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Checkout PBF | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| lfs: true | ||
|
|
||
| - name: Download Valhalla wheel | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: valhalla-wheel-${{ steps.sanitize.outputs.ref_slug }} | ||
| path: /tmp/valhalla-dist | ||
|
|
||
| - name: Install Valhalla from wheel | ||
| run: pip install /tmp/valhalla-dist/*.whl | ||
|
|
||
| - name: Build graph tiles | ||
| run: | | ||
| valhalla_build_config \ | ||
| --mjolnir-tile-dir "$(pwd)/valhalla_tiles" \ | ||
| --mjolnir-admin "$(pwd)/valhalla_tiles/admins.sqlite" \ | ||
| > valhalla.json | ||
| python -m valhalla valhalla_build_admins --config valhalla.json data/liechtenstein_graph.osm.pbf | ||
| python -m valhalla valhalla_build_tiles --config valhalla.json data/liechtenstein_graph.osm.pbf | ||
| find valhalla_tiles -name "*.gph" | wc -l | ||
|
Comment on lines
+47
to
+49
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of uploading all files individually, we should use |
||
|
|
||
| - name: Upload tiles artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: valhalla-tiles-${{ steps.sanitize.outputs.ref_slug }} | ||
| path: valhalla_tiles/ | ||
| retention-days: 90 | ||
|
|
||
| - name: Print run ID | ||
| run: echo "tiles_run_id = ${{ github.run_id }}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| name: Build Valhalla | ||
|
|
||
| on: | ||
| workflow_call: | ||
| inputs: | ||
| valhalla_ref: | ||
| description: "Valhalla git ref to build (branch, tag, or commit SHA)" | ||
| type: string | ||
| required: true | ||
| outputs: | ||
| wheel_artifact: | ||
| description: "Name of the uploaded wheel artifact" | ||
| value: ${{ jobs.build.outputs.wheel_artifact }} | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| wheel_artifact: ${{ steps.set-output.outputs.wheel_artifact }} | ||
|
|
||
| steps: | ||
| - name: Sanitize ref for artifact name | ||
| id: sanitize | ||
| run: echo "ref_slug=$(echo '${{ inputs.valhalla_ref }}' | tr '/' '-')" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Checkout Valhalla | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: valhalla/valhalla | ||
| ref: ${{ inputs.valhalla_ref }} | ||
| submodules: recursive | ||
| path: valhalla-src | ||
|
|
||
| - name: Restore ccache | ||
| uses: tespkg/actions-cache/restore@v1 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually: let's not use S3 for ccache after all. in the end we'll rarely build this, so we can use github's provided cache, it's 10 GB, more than comfortable for our project. |
||
| with: | ||
| endpoint: ${{ secrets.HETZNER_S3_ENDPOINT }} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where did you get this secret from? |
||
| accessKey: ${{ secrets.HETZNER_S3_ACCESS_KEY }} | ||
| secretKey: ${{ secrets.HETZNER_S3_ACCESS_SECRET }} | ||
| bucket: rad-cache | ||
| path: ~/.cache/ccache | ||
| key: ccache-valhalla-${{ steps.sanitize.outputs.ref_slug }}-${{ github.sha }} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. S3 is not unlimited, it's my private instance which costs me;) pls follow the valhalla conventions |
||
| restore-keys: | | ||
| ccache-valhalla-${{ steps.sanitize.outputs.ref_slug }}- | ||
| ccache-valhalla-master- | ||
|
|
||
| - name: Install system dependencies | ||
| run: bash valhalla-src/scripts/install-linux-deps.sh | ||
|
Comment on lines
+47
to
+48
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. first install, then restore cache |
||
|
|
||
| - name: Install Python build dependencies | ||
| run: pip install scikit-build-core pyproject-metadata setuptools-scm pybind11 | ||
|
|
||
|
Comment on lines
+50
to
+52
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is pretty bad for maintenance: whenever we add a build dependency, we now have to update two places! remove |
||
| - name: Build wheel | ||
| run: | | ||
| cd valhalla-src | ||
| pip wheel . --no-build-isolation --wheel-dir /tmp/valhalla-dist \ | ||
| -Ccmake.build-type=Release \ | ||
| -Ccmake.define.ENABLE_PYTHON_BINDINGS=ON \ | ||
| -Ccmake.define.ENABLE_TESTS=OFF \ | ||
| -Ccmake.define.ENABLE_SERVICES=OFF | ||
|
Comment on lines
+56
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this'll currently work well for caching. ccache uses a lot of heuristics to invalidate cache hits and I'm quite sure compilation commands might use e.g. this is just an educated guess. can you make sure that doesn't happen currently by simply executing this command twice on your local machine (of course with ccache installed)? you'll need to watch ccache hits/stats before & after the second run.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and what happened to #12 (comment)? the way you use it now would need |
||
|
|
||
| - name: Save ccache | ||
| if: always() | ||
| uses: tespkg/actions-cache/save@v1 | ||
| with: | ||
| endpoint: ${{ secrets.HETZNER_S3_ENDPOINT }} | ||
| accessKey: ${{ secrets.HETZNER_S3_ACCESS_KEY }} | ||
| secretKey: ${{ secrets.HETZNER_S3_ACCESS_SECRET }} | ||
| bucket: rad-cache | ||
| path: ~/.cache/ccache | ||
| key: ccache-valhalla-${{ steps.sanitize.outputs.ref_slug }}-${{ github.sha }} | ||
|
|
||
| - name: Upload wheel artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: valhalla-wheel-${{ steps.sanitize.outputs.ref_slug }} | ||
| path: /tmp/valhalla-dist/*.whl | ||
| retention-days: 7 | ||
|
Comment on lines
+73
to
+78
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I still don't understand why we're uploading anything to artifacts. here at least: this whole "build-valhalla.yml" can be a re-usable part of a workflow. like a building block, which is callabable by actual workflows like build-tiles.yml. that's a combination of |
||
|
|
||
| - name: Set output | ||
| id: set-output | ||
| run: echo "wheel_artifact=valhalla-wheel-${{ steps.sanitize.outputs.ref_slug }}" >> $GITHUB_OUTPUT | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| name: Routing Regression | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| valhalla_ref_old: | ||
| description: "Baseline Valhalla ref (the 'before')" | ||
| default: master | ||
| required: true | ||
| valhalla_ref_new: | ||
| description: "New Valhalla ref to test (the 'after' — branch, tag, or SHA)" | ||
| required: true | ||
| tiles_run_id: | ||
| description: "Run ID from a successful build-tiles.yml run" | ||
| required: true | ||
|
Comment on lines
+13
to
+15
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is pretty brittle and awkward to hunt down. |
||
|
|
||
| jobs: | ||
| build-valhalla-old: | ||
| uses: ./.github/workflows/build-valhalla.yml | ||
| with: | ||
| valhalla_ref: ${{ inputs.valhalla_ref_old }} | ||
| secrets: inherit | ||
|
|
||
| build-valhalla-new: | ||
| uses: ./.github/workflows/build-valhalla.yml | ||
| with: | ||
| valhalla_ref: ${{ inputs.valhalla_ref_new }} | ||
| secrets: inherit | ||
|
|
||
|
Comment on lines
+18
to
+29
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here we're building both SHAs again. once for build-tiles.yml, now for route regression test. we only need to build valhalla (bindings) once, if we do it right. that's a good 20 mins of CI time saved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really wonder why you build valhalla here again? I mean I guess the whole point of you pushing the wheel artifact from |
||
| run-routes: | ||
| runs-on: ubuntu-latest | ||
| needs: [build-valhalla-old, build-valhalla-new] | ||
| strategy: | ||
| matrix: | ||
| router: | ||
| - name: old | ||
| ref: ${{ inputs.valhalla_ref_old }} | ||
| - name: new | ||
| ref: ${{ inputs.valhalla_ref_new }} | ||
|
|
||
| steps: | ||
| - name: Sanitize ref for artifact name | ||
| id: sanitize | ||
| run: echo "ref_slug=$(echo '${{ matrix.router.ref }}' | tr '/' '-')" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Checkout RAD | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Checkout RAD-data | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: valhalla/RAD-data | ||
| path: RAD-data | ||
| token: ${{ secrets.RAD_DATA_TOKEN }} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, it's the same token. it's the 2nd time I'm reviewing this line and asking you to change it. please make sure already fixed things stay fixed! |
||
|
|
||
| - name: Download Valhalla wheel | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: valhalla-wheel-${{ steps.sanitize.outputs.ref_slug }} | ||
| path: /tmp/valhalla-dist | ||
|
|
||
| - name: Install Valhalla from wheel | ||
| run: pip install /tmp/valhalla-dist/*.whl | ||
|
|
||
|
Comment on lines
+62
to
+64
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do want to control the python version this runs with. |
||
| - name: Download tiles from build-tiles run | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| gh run download ${{ inputs.tiles_run_id }} \ | ||
| --repo "${{ github.repository }}" \ | ||
| --pattern "valhalla-tiles-*" \ | ||
| --dir valhalla_tiles | ||
| shopt -s dotglob | ||
| mv valhalla_tiles/valhalla-tiles-*/* valhalla_tiles/ 2>/dev/null || true | ||
|
Comment on lines
+65
to
+74
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not priority, but worth mentioning: one thing we said we want to look out for is future compatibility with e.g. scenario old graph/new graph. currently it's fixed to using a single graph, whatever don't try hard to keep that requirement in mind for the next round of edits. we can deal with more scenarios once we get there. I just needed to mention it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also don't sync via artifacts. let the graphs be uploaded to S3, that's a much more idiomatic place for them, especially the master graph. PR graphs should be uploaded to S3 as well, they'll be cleaned once a PR closes. |
||
|
|
||
| - name: Run route requests | ||
| run: | | ||
| valhalla_build_config \ | ||
| --mjolnir-tile-dir "$(pwd)/valhalla_tiles" \ | ||
| > /tmp/valhalla.json | ||
|
Comment on lines
+78
to
+80
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pass the dir (after your next edit: the tar of the dir) to |
||
| python3 scripts/run_routes.py \ | ||
| --config /tmp/valhalla.json \ | ||
| --requests RAD-data/requests/requests.jsonl \ | ||
| --output /tmp/responses-${{ matrix.router.name }}.jsonl | ||
|
|
||
| - name: Upload responses artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: responses-${{ matrix.router.name }} | ||
| path: /tmp/responses-${{ matrix.router.name }}.jsonl | ||
| retention-days: 7 | ||
|
Comment on lines
+86
to
+91
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is literally the only thing we want uploaded to GH artifacts. |
||
|
|
||
| diff-and-store: | ||
| runs-on: ubuntu-latest | ||
| needs: run-routes | ||
| permissions: | ||
| contents: write | ||
|
|
||
| steps: | ||
| - name: Checkout RAD | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Checkout RAD-data | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: valhalla/RAD-data | ||
| path: RAD-data | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Download responses | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| pattern: responses-* | ||
| path: /tmp/responses | ||
| merge-multiple: false | ||
|
|
||
| - name: Compute diff and push to RAD-data | ||
| run: | | ||
| python3 scripts/diff_responses.py \ | ||
| --old /tmp/responses/responses-old/responses-old.jsonl \ | ||
| --new /tmp/responses/responses-new/responses-new.jsonl \ | ||
| --old-ref "${{ inputs.valhalla_ref_old }}" \ | ||
| --new-ref "${{ inputs.valhalla_ref_new }}" \ | ||
| --output RAD-data/diffs/${{ github.run_id }}.json | ||
| cd RAD-data | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git add diffs/${{ github.run_id }}.json | ||
| git commit -m "regression: ${{ inputs.valhalla_ref_old }} vs ${{ inputs.valhalla_ref_new }} (run ${{ github.run_id }})" | ||
| git push | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| #!/usr/bin/env python3 | ||
| """ | ||
| Usage: | ||
| python3 scripts/diff_responses.py \ | ||
| --old /tmp/responses/responses-old/responses-old.jsonl \ | ||
| --new /tmp/responses/responses-new/responses-new.jsonl \ | ||
| --old-ref master \ | ||
| --new-ref feature-branch \ | ||
| --output RAD-data/diffs/12345678.json | ||
| """ | ||
|
|
||
| import argparse | ||
| import json | ||
| from datetime import UTC, datetime | ||
|
|
||
|
|
||
| def load_jsonl(path): | ||
| return [json.loads(line) for line in open(path)] | ||
|
|
||
|
|
||
| def diff_entry(old, new): | ||
| # TODO: expand with geometry comparison, maneuver diffs, etc. | ||
| old_dur = old["response"].get("trip", {}).get("summary", {}).get("time", None) | ||
| new_dur = new["response"].get("trip", {}).get("summary", {}).get("time", None) | ||
| return { | ||
| "request": old["request"], | ||
| "old_duration": old_dur, | ||
| "new_duration": new_dur, | ||
| "duration_delta_s": (new_dur - old_dur) if (old_dur and new_dur) else None, | ||
| "old_status": old["status"], | ||
| "new_status": new["status"], | ||
| } | ||
|
|
||
|
|
||
| def main(): | ||
| p = argparse.ArgumentParser() | ||
| p.add_argument("--old", required=True) | ||
| p.add_argument("--new", required=True) | ||
| p.add_argument("--old-ref", required=True) | ||
| p.add_argument("--new-ref", required=True) | ||
| p.add_argument("--output", required=True) | ||
| args = p.parse_args() | ||
|
|
||
| old_rows = load_jsonl(args.old) | ||
| new_rows = load_jsonl(args.new) | ||
|
|
||
| diffs = [diff_entry(o, n) for o, n in zip(old_rows, new_rows, strict=False)] | ||
| changed = [d for d in diffs if d["duration_delta_s"] not in (None, 0)] | ||
|
|
||
| result = { | ||
| "meta": { | ||
| "old_ref": args.old_ref, | ||
| "new_ref": args.new_ref, | ||
| "generated_at": datetime.now(UTC).isoformat(), | ||
| "total_requests": len(diffs), | ||
| "changed_routes": len(changed), | ||
| }, | ||
| "diffs": diffs, | ||
| } | ||
|
|
||
| with open(args.output, "w") as f: | ||
| json.dump(result, f, indent=2) | ||
|
|
||
| print(f"Done: {len(changed)}/{len(diffs)} routes changed") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| #!/usr/bin/env python3 | ||
| """ | ||
| Usage: | ||
| python3 scripts/run_routes.py \ | ||
| --config /tmp/valhalla.json \ | ||
| --requests RAD-data/requests/requests.jsonl \ | ||
| --output /tmp/responses-old.jsonl | ||
| """ | ||
|
|
||
| import argparse | ||
| import json | ||
|
|
||
| import valhalla | ||
|
|
||
|
|
||
| def main(): | ||
| p = argparse.ArgumentParser() | ||
| p.add_argument("--config", required=True) | ||
| p.add_argument("--requests", required=True) | ||
| p.add_argument("--output", required=True) | ||
| args = p.parse_args() | ||
|
|
||
| config = args.config | ||
| actor = valhalla.Actor(config) | ||
|
|
||
| with open(args.requests) as req_f, open(args.output, "w") as out_f: | ||
| for line in req_f: | ||
| request = json.loads(line) | ||
| try: | ||
| response = json.loads(actor.route(json.dumps(request))) | ||
| status = "ok" | ||
| except Exception as e: | ||
| response = {"error": str(e)} | ||
| status = "error" | ||
| out_f.write(json.dumps({"request": request, "response": response, "status": status}) + "\n") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
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.
ha, you even use it here. but again, it's a redundant job, see #12 (comment). you do that in
build-tilesbelow without any artifacts.