Conversation
ppinchuk
added
p-medium
There was a problem hiding this comment.
Pull request overview
This PR extends reVRt’s Rust routing swap/routing-layer storage so callers (notably Python and the Python CLI) can control where the routing-layer Zarr cache is written, and optionally persist that cache as an inspectable output artifact.
Changes:
- Add an optional
routing_layer_out_fp/swap_fppath plumbed from Python → PyO3 → Rust routing → Rust dataset swap store. - Add CLI support for
save_routing_layer, persisting routing-layer outputs underout_dir/extra_outputs. - Add tests covering routing-layer persistence and argument passthrough; add
python-slugifydependency for stable filenames.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rust/integration_tests.rs | Updates Rust integration tests for new resolve argument ordering and swap_fp=None. |
| tests/python/unit/test_rust_bindings.py | Adds unit test ensuring RouteFinder writes routing-layer Zarr to a requested path and is readable/consistent. |
| tests/python/unit/routing/test_routing_base.py | Adds test verifying routing_layer_out_fp is passed through to RouteFinder from BatchRouteProcessor. |
| tests/python/unit/routing/cli/test_routing_cli_point_to_point.py | Adds test for compute_lcp_routes(..., save_routing_layer=True) producing a saved .zarr output. |
| tests/python/unit/routing/cli/test_routing_cli_point_to_feature.py | Adds analogous test for point-to-feature CLI routing-layer persistence. |
| tests/python/integration/test_rust_bindings_integration.py | Adds integration test validating routing-layer output persistence for LayeredFile inputs. |
| revrt/routing/cli/utilities.py | New helper/context manager to create temp routing-layer directories and persist them with coordinates/CRS. |
| revrt/routing/cli/point_to_point.py | Adds save_routing_layer option and passes it through to the shared routing runner. |
| revrt/routing/cli/point_to_feature.py | Adds save_routing_layer option and passes it through to the shared routing runner. |
| revrt/routing/cli/base.py | Wires save_routing_layer into run_lcp and routes routing-layer temp/persist handling via the new utility. |
| revrt/routing/base.py | Extends BatchRouteProcessor.process() / routing execution to pass routing_layer_out_fp into Rust RouteFinder. |
| pyproject.toml | Adds runtime dependency on python-slugify for deterministic output naming. |
| pixi.lock | Updates lockfile to include python-slugify and text-unidecode. |
| crates/revrt/src/routing/scenario.rs | Threads optional swap_fp into Scenario::new() and dataset creation. |
| crates/revrt/src/routing/mod.rs | Threads optional swap_fp into Routing::new() and ParRouting::new(). |
| crates/revrt/src/lib.rs | Updates resolve / generator plumbing for swap_fp; adjusts internal tests/bench calls. |
| crates/revrt/src/ffi/mod.rs | Exposes routing_layer_out_fp in find_paths and RouteFinder PyO3 signatures; forwards to Rust core. |
| crates/revrt/src/dataset/mod.rs | Supports swap storage at a user-provided filesystem path; improves swap dataset metadata (dimension names). |
| crates/cli/src/main.rs | Updates native CLI invocation of resolve for the new signature and swap_fp=None. |
Comments suppressed due to low confidence (2)
revrt/routing/cli/point_to_point.py:318
- Docstring uses single-backtick inline literals for parameter names (e.g.,
system_mem_limit_gb,memory_utilization_limit). In reST/Sphinx, parameter names/literals should use double backticks (system_mem_limit_gb) for consistent monospace rendering (matches the rest of this docstring).
Fraction of `system_mem_limit_gb` to utilize for routing. Should
be a value between 0 and 1. By default, ``0.9``.
system_mem_limit_gb : int or float, default=5
Maximum amount of system memory (in GB) to utilize for routing.
This is used in conjunction with `memory_utilization_limit` to
determine the memory limit for routing. By default, ``5`` GB.
revrt/routing/cli/point_to_feature.py:435
- Docstring uses single-backtick inline literals for parameter names (e.g.,
system_mem_limit_gb,memory_utilization_limit). In reST/Sphinx, parameter names/literals should use double backticks (system_mem_limit_gb) for consistent monospace rendering (matches the rest of this docstring).
Fraction of `system_mem_limit_gb` to utilize for routing. Should
be a value between 0 and 1. By default, ``0.9``.
system_mem_limit_gb : int or float, default=5
Maximum amount of system memory (in GB) to utilize for routing.
This is used in conjunction with `memory_utilization_limit` to
determine the memory limit for routing. By default, ``5`` GB.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (78.78%) is below the target coverage (93.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
==========================================
- Coverage 99.34% 98.63% -0.72%
==========================================
Files 25 26 +1
Lines 2764 2857 +93
Branches 320 331 +11
==========================================
+ Hits 2746 2818 +72
- Misses 9 25 +16
- Partials 9 14 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "Initializing a temporary swap dataset at {:?}", | ||
| tmp_path.path() | ||
| ); | ||
| let tmp_path = tempfile::TempDir::new() |
There was a problem hiding this comment.
Shouldn't this go within the None? If the path is specified, we don't need the TempDir, so no problem if it is not created, right? Oh, but in that case, tmp_path would go out of scope by the end of the swap_fp and hence dropped before it could be used? Was that the reason?
There was a problem hiding this comment.
yes, that's exactly right
| } | ||
|
|
||
| impl Dataset { | ||
| pub(super) fn open<P: AsRef<std::path::Path>>( |
There was a problem hiding this comment.
Since we can't silently skip variables with optional as we do in Python, what about having a variation of this command that requires a path and another that doesn't? What do we expect to be the most common, the temporary or with path defined?
One option would be open_with_path(..., path), and open(...) which creates the TmpDir and calls open_with_path(). With that we keep the current signature, DRY, and open the option for a path defined.
| } | ||
|
|
||
| impl Scenario { | ||
| pub(super) fn new<P: AsRef<std::path::Path>>( |
There was a problem hiding this comment.
If you agreed with the other suggestion, it would make sense the same here. Something like new_with_path.
|
Also, we should do a patch upgrade after this PR. |
In some cases (i.e. Kestrel) it is important for users (specifically from Python) to be able to specify the location of the temp swap storage. This allows users to select the most performant hardware and also potentially save the file after execution to be able to examine the routing layers. This PR allows Python to pass down a temp dir for the swap file and also allows python CLI users to request that the routing layer file is stored as an output.