Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .github/actions/build-tiles/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: Build Tiles
description: >
Builds a routable Valhalla graph extract from a PBF, using the Valhalla
install already present on the runner. Outputs a single tile_extract tar.

inputs:
pbf_path:
description: "Path to the OSM PBF to build the graph from"
required: true
tile_dir:
description: "Working directory to build raw tiles into before bundling"
required: false
default: "valhalla_tiles"

outputs:
tile_extract:
description: "Absolute path to the built, uncompressed tile extract tar"
value: ${{ steps.extract.outputs.tile_extract }}

runs:
using: composite
steps:
- name: Build config, admins, and tiles
shell: bash
run: |
mkdir -p "${{ inputs.tile_dir }}"
valhalla_build_config \
--mjolnir-tile-dir "${{ inputs.tile_dir }}" \
--mjolnir-admin "${{ inputs.tile_dir }}/admins.sqlite" \
--mjolnir-tile-extract "${{ inputs.tile_dir }}/tiles.tar" \
> /tmp/valhalla-build.json
valhalla_build_admins -c /tmp/valhalla-build.json "${{ inputs.pbf_path }}"
valhalla_build_tiles -c /tmp/valhalla-build.json "${{ inputs.pbf_path }}"

- name: Bundle tiles into a single extract
id: extract
shell: bash
run: |
valhalla_build_extract -c /tmp/valhalla-build.json -v
tile_count=$(find "${{ inputs.tile_dir }}" -name "*.gph" | wc -l)
echo "built $tile_count tile(s)"
test -f "${{ inputs.tile_dir }}/tiles.tar"
echo "tile_extract=$(pwd)/${{ inputs.tile_dir }}/tiles.tar" >> "$GITHUB_OUTPUT"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it's fairly meaningless to have this as a separate build step. nothing is different here. I'd prefer to push this bash code into e.g. ./.github/workflows/scripts (make sure scripts isn't somehow reserved!). then we can run shell-check on it and proper syntax hightlighting in IDEs etc.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Moved the bundle logic into .github/workflows/scripts/build_extract.sh and merged both steps into one. Verified with shellcheck -clean.

68 changes: 68 additions & 0 deletions .github/actions/build-valhalla/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
name: Build Valhalla
description: >
Builds Valhalla from source as a Python wheel and installs it into the
current job's environment.

inputs:
valhalla_ref:
description: "Valhalla git ref to build (branch, tag, or commit SHA)"
required: true
python_version:
description: "Python version to build and install the wheel against"
required: false
default: "3.12"

runs:
using: composite
steps:
- name: Set up Python
uses: actions/setup-python@v6
with:
python-version: ${{ inputs.python_version }}

- name: Sanitize ref for cache key
id: sanitize
shell: bash
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: Install system dependencies
shell: bash
run: bash valhalla-src/scripts/install-linux-deps.sh

- name: Restore ccache
uses: actions/cache/restore@v4
with:
path: ~/.cache/ccache
key: ccache-valhalla-${{ steps.sanitize.outputs.ref_slug }}
restore-keys: |
ccache-valhalla-master

# pyproject.toml sets Release/ENABLE_PYTHON_BINDINGS/ENABLE_TESTS defaults
- name: Build wheel
shell: bash
run: |
cd valhalla-src
pip wheel . --wheel-dir /tmp/valhalla-dist \
-Cbuild-dir=/tmp/valhalla-build \
-Ccmake.define.ENABLE_TOOLS=ON \
-Ccmake.define.ENABLE_SERVICES=OFF \
-Ccmake.define.ENABLE_CCACHE=ON

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is the kind of thing I expected you to get from the old PR. it's not the most relevant thing, but this is at least the 3rd time that I need to flag it in a review!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed ENABLE_TOOLS=ON (default ON, not needed explicitly) and added ENABLE_GEOTIFF=OFF and ENABLE_LZ4=OFF as suggested. Kept only flags that differ from CMakeLists.txt defaults plus ENABLE_CCACHE=ON explicitly since it's important for our caching.


- name: Save ccache
if: always()
uses: actions/cache/save@v4
with:
path: ~/.cache/ccache
key: ccache-valhalla-${{ steps.sanitize.outputs.ref_slug }}

- name: Install wheel
shell: bash
run: pip install /tmp/valhalla-dist/*.whl
109 changes: 109 additions & 0 deletions .github/workflows/routing-regression.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
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

# queue concurrent runs rather than cancel
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: false

jobs:
regression:
runs-on: ubuntu-latest
strategy:
matrix:
include:
- label: old
ref: ${{ inputs.valhalla_ref_old }}
- label: new
ref: ${{ inputs.valhalla_ref_new }}

steps:
- name: Checkout RAD
uses: actions/checkout@v6
with:
lfs: true

- name: Checkout RAD-data
uses: actions/checkout@v6
with:
repository: valhalla/RAD-data
path: RAD-data

- name: Build Valhalla at ${{ matrix.label }} ref
uses: ./.github/actions/build-valhalla
with:
valhalla_ref: ${{ matrix.ref }}
python_version: "3.12"

- name: Build tiles for ${{ matrix.label }} ref
id: tiles
uses: ./.github/actions/build-tiles
with:
pbf_path: data/liechtenstein_graph.osm.pbf

Comment on lines +48 to +53

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there's a mistake hidden here: currently we'd build graphs for both SHAs. but we said we'd only want a single graph (e.g. master) right now. otherwise we can't tell what the problem is (graph build or service runtime?). that's the whole point of the "scenarios" in the initial issue, the base for this whole project.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so for now: 1 single graph. 2 different service runtimes (the pyvalhalla stuff below). ideally in a way so that it'll be easier to add future scenarios, but I think that'll need a bigger re-write than necessary and we'll park that for later.

feel free to just use artifacts for the master graph for now. we'll revisit later.

- name: Run route requests
run: |
python3 scripts/run_routes.py \
--tile-extract "${{ steps.tiles.outputs.tile_extract }}" \
--requests RAD-data/requests/requests.jsonl \
--output /tmp/responses-${{ matrix.label }}.jsonl

# responses are only needed for the current run's diff-and-store job, 7 days is generous
- name: Upload responses artifact
uses: actions/upload-artifact@v4
with:
name: responses-${{ matrix.label }}
path: /tmp/responses-${{ matrix.label }}.jsonl
retention-days: 7

diff-and-store:
runs-on: ubuntu-latest
needs: regression
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

- name: Download responses
uses: actions/download-artifact@v4
with:
pattern: responses-*
path: /tmp/responses
merge-multiple: false

# Debug step
- name: List downloaded artifacts
run: find /tmp/responses -type f

- 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 https://x-access-token:${{ secrets.RAD_DATA_TOKEN }}@github.com/valhalla/RAD-data.git

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there a way to get rid of the input token? since we control both repos? some permissions one can set maybe?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not with just workflow permissions, those are scoped to the repo the workflow runs in. Options I see: a GitHub App with org-level install, or keeping the PAT.
Ano. option that avoids a PAT entirely: flip the push direction. RAD-data exposes a workflow_call workflow that RAD triggers, RAD-data then writes to itself using its own GITHUB_TOKEN. Needs RAD-data's Actions settings set to "Accessible from repositories in the organization" and a reusable workflow added there. Alternatively, keeping the PAT is simpler for now. What do you think?

118 changes: 118 additions & 0 deletions architecture.md

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looking at its current state, it's too busy with words. but it's good to have it started, we'll need to change things around anyways in the future.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Noted, will trim it down in a follow-up once the architecture settles.

Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
# RAD — Routing Regression Pipeline

```mermaid
flowchart TD
TRIGGER(["`**workflow_dispatch**
valhalla_ref_old · valhalla_ref_new`"])

TRIGGER -->|matrix: label=old| OLD_JOB
TRIGGER -->|matrix: label=new| NEW_JOB

subgraph OLD_JOB ["old ref runner (ubuntu-latest)"]
direction TB
O1["① build-valhalla
composite action
─────────────────
checkout valhalla at old ref
install-linux-deps.sh
restore ccache ← actions/cache
pip wheel . -Cbuild-dir=/tmp/valhalla-build
save ccache → actions/cache
pip install wheel"]

O2["② build-tiles
composite action
─────────────────
valhalla_build_config
valhalla_build_admins
valhalla_build_tiles
valhalla_build_extract → tiles.tar"]

O3["③ run routes
─────────────────
run_routes.py
pyvalhalla Actor in-process
→ responses-old.jsonl"]

O4[/"④ upload artifact
responses-old.jsonl"/]

O1 --> O2 --> O3 --> O4
end

subgraph NEW_JOB ["new ref runner (ubuntu-latest)"]
direction TB
N1["① build-valhalla
composite action
─────────────────
checkout valhalla at new ref
install-linux-deps.sh
restore ccache ← actions/cache
pip wheel . -Cbuild-dir=/tmp/valhalla-build
save ccache → actions/cache
pip install wheel"]

N2["② build-tiles
composite action
─────────────────
valhalla_build_config
valhalla_build_admins
valhalla_build_tiles
valhalla_build_extract → tiles.tar"]

N3["③ run routes
─────────────────
run_routes.py
pyvalhalla Actor in-process
→ responses-new.jsonl"]

N4[/"④ upload artifact
responses-new.jsonl"/]

N1 --> N2 --> N3 --> N4
end

O4 --> DIFF
N4 --> DIFF

subgraph DIFF ["diff-and-store (needs: regression)"]
direction TB
D1["download both artifacts
─────────────────
responses-old.jsonl
responses-new.jsonl"]

D2["diff_responses.py
─────────────────
geometry delta · duration delta
instruction diff · severity score"]

D3["git commit → valhalla/RAD-data
─────────────────
diffs/{run_id}.json
push via RAD_DATA_TOKEN"]

D1 --> D2 --> D3
end

CACHE[("actions/cache
~/.cache/ccache
key: ccache-valhalla-{ref_slug}")]

RADDATA[("valhalla/RAD-data
diffs/{run_id}.json")]

O1 <-->|restore / save| CACHE
N1 <-->|restore / save| CACHE
D3 -->|git push| RADDATA
```

## Storage layout

| Data | Where | Lifetime |
|------|--------|----------|
| ccache object files | GitHub `actions/cache` | until evicted (7-day LRU) |
| pyvalhalla wheel (16MB) | `/tmp/valhalla-dist` on runner | single job |
| graph tiles tar | `/tmp/valhalla_tiles` on runner | single job |
| route responses | GHA artifact | 7 days |
| diff results | `valhalla/RAD-data` git commit | permanent |
Loading