feat(bus): Keep original values for gtfs_arrival_dt and gtfs_departure_dt - #795
runkelcorey wants to merge 13 commits into
Conversation
LCOV of commit
|
There was a problem hiding this comment.
🟡 Changes recommended
read_vehicle_positions can return an unbound local after an exception, which can mask the real read failure and break the pipeline.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the bus performance pipeline to preserve original GTFS-RT timestamp components (gtfs_arrival_dt / gtfs_departure_dt) by deferring derivation of stop_arrival_dt / stop_departure_dt until the metrics stage, and adjusts validation + tests accordingly.
Changes:
- Stop overwriting GTFS arrival timestamps during the RT↔schedule join; derive
stop_arrival_dt/stop_departure_dtlater in metrics. - Move/adjust validation for “arrival required when GTFS-RT data exists” from
BusEventstoBusPerformanceMetrics, updating tests. - Refactor GTFS-RT parquet reading toward a Polars
LazyFramepipeline and bump the publishedbus_eventsS3 version.
File summaries
| File | Description |
|---|---|
| tests/bus_performance_manager/test_events_metrics.py | Adds/adjusts metrics-level validation tests and updates existing metrics tests for new required columns. |
| tests/bus_performance_manager/test_events_joined.py | Removes the join-stage test for final-stop GTFS arrival validation (logic moved downstream). |
| src/lamp_py/runtime_utils/remote_files.py | Bumps published bus_events dataset version from 1.2 to 1.3. |
| src/lamp_py/bus_performance_manager/events_metrics.py | Adds a new rule (has_arrival_dt) and refines stop arrival/departure derivation to use GTFS in-transit timestamps without mutating originals. |
| src/lamp_py/bus_performance_manager/events_joined.py | Removes join-stage logic that backfilled/overwrote gtfs_arrival_dt using gtfs_last_in_transit_dt. |
| src/lamp_py/bus_performance_manager/events_gtfs_rt.py | Changes parquet ingest to return a LazyFrame, removes the pyarrow fallback, and updates downstream types. |
| src/lamp_py/bus_performance_manager/init.py | Trims trailing whitespace in the module docstring. |
Review details
Suppressed comments (3)
src/lamp_py/bus_performance_manager/events_gtfs_rt.py:124
- In
read_vehicle_positions, if_read_with_polarsraises,vehicle_positionsis never assigned; the function then returns an unbound local, masking the original read error with anUnboundLocalError. Re-raise after logging (and ensurelog_complete()still runs).
try:
vehicle_positions = _read_with_polars(service_date, gtfs_rt_files, bus_routes)
except Exception as e:
logger.log_failure(e)
logger.log_complete()
return vehicle_positions
src/lamp_py/bus_performance_manager/events_gtfs_rt.py:43
- The
_read_with_polarsdocstring still references a pyarrow implementation and has typos (e.g., “enginer”). Since the pyarrow fallback was removed and the function now returns aLazyFrame, the docstring should be updated to match current behavior.
This issue also appears on line 118 of the same file.
"""
Read RT_VEHICLE_POSITIONS parquet files with polars engine
Polars engine appears to be faster and use less memory than pyarrow enginer, but is not as
compatible with all parquet file formats as pyarrow engine
"""
src/lamp_py/bus_performance_manager/events_metrics.py:223
- This inline comment is incorrect:
shift(-1)pulls the value from the next stop (not the previous one). Updating it will prevent confusion when reasoning about how departure times are derived.
.shift(-1)
.over(partition_by=["trip_id", "tm_pullout_id"], order_by="stop_sequence"),
)
) # use the first in transit dt from the previous stop)
.when(pl.col("point_type").eq(pl.lit("end"))) # endpoints
- Files reviewed: 6/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LCOV of commit
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Working group needs to be able to reconstruct the
stop_arrival_dtandstop_departure_dtfrom its components.What changes does this PR propose?
Refactors to defer creating
stop_arrival_dtandstop_departure_dtuntil the last stage.How were these changes validated?
What questions should reviewers consider?
None.