Keep CARLA's world clock in step with alpasim's control loop#8
Merged
Conversation
`CarlaSession.tick_until` used ceil(delta / fixed_delta) with a "always tick at least once" floor, and then stored the alpasim-requested target time as `last_time_query_us`. That combination lets the two clocks drift: every RPC where the requested delta is not an exact multiple of `fixed_delta_seconds` either over-ticks CARLA (ceil) or ticks a full step for a sub-step delta (the `max(1, ...)` floor), while the bookkeeping records the caller's intended time. Over a rollout the divergence compounds and traffic snapshots no longer correspond to the timestamp alpasim thinks it queried. Track CARLA's own world clock instead: - `tick_until` now requires `target_time_us` to land on a `fixed_delta` boundary and advances by exactly `delta // step` ticks. Delta == 0 and past targets are explicit no-ops so idempotent queries don't silently advance the world. - Misalignment raises `ValueError` so future configs can't reintroduce the drift silently. To let alpasim own the tick cadence end-to-end, add `tick_interval_us` to `TrafficSessionRequest` and have Runtime forward its `control_timestep_us` in `TrafficSessionConfig`. The trafficsim server now resolves the effective `fixed_delta_seconds` (request > scenario > default) *before* constructing `CarlaSession`, so `session.open()` applies the correct value to CARLA's world settings. The scenario runner's post-open override was removed because it left `session.fixed_delta_seconds` inconsistent with the world CARLA was actually simulating. Includes unit tests covering aligned advance, no-op-on-zero-delta, no-op for past targets, misalignment rejection, and constructor-driven step selection. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to d70425d addressing /simplify review findings: - Fix critical first-call seeding bug in CarlaSession.tick_until: the previous default of last_time_query_us=0 forced every real-log rollout (target_time_us in the trillions) to fail the alignment check on the first tick. Switch to an Optional[int] sentinel and seed the clock on first call without ticking. - Make TrafficSessionConfig.control_timestep_us required so future callers cannot silently drop the tick-interval wiring. - Replace the six near-clone tick_until tests with a parametrized suite plus a dedicated first-call regression test. - Simplify server.start_session fixed_delta_seconds resolution by dropping the kwargs-dict pattern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Superseded: reworking to move CARLA world.tick() ownership from trafficsim into the physics container on the same branch. Will reopen after the refactor. |
The physics container now owns CARLA's world settings and drives the world clock. It opens its own CARLA client on start_session, calls apply_settings(synchronous_mode=True, fixed_delta_seconds=X) once per rollout, and ticks the world as a side-effect of ground_intersection on the ego-facing call each control step (via the new advance_world_to_us field). Trafficsim keeps its client for spawning and reading/writing poses but no longer touches world.tick() or world settings — the name of the container matches the responsibility. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- server.start_session: check ALREADY_EXISTS before clock.open() and hold the lock across open+insert, so a duplicate/racing RPC no longer leaks a CARLA client and cannot silently override world settings. - server.ground_intersection: don't overwrite context.abort()'s status code in the generic exception handler — RpcError now propagates with the intended FAILED_PRECONDITION instead of being flattened to UNKNOWN. - carla_clock: parameterize by integer tick_interval_us instead of a float fixed_delta_seconds so the alignment check doesn't drift by ±1us on non-terminating fractions. - carla_clock: drop the unused _prev_settings field and its comment claiming close() restores it — close() already just re-applies async mode, which is what we want. - carla_clock.advance_to: raise a clear RuntimeError if called on a closed clock instead of AttributeError on None.tick(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The alpasim topology co-locates the CARLA server inside the physics container (trafficsim reaches it as physics-0:2000; physics reaches it as localhost:2000). Both the CLI flags and the pass-through constructor args were dead configuration surface — remove them and pin the CARLA endpoint to localhost:2000 inside CarlaClock, which is the only place that opens the connection. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move all CARLA-touching code out of src/ so alpasim service code works without CARLA installed. Physics and trafficsim servers now live in docker/carla/carla_physics_server and docker/carla/carla_trafficsim_server, built into the CARLA-bundled images. src/physics/alpasim_physics keeps the CARLA-free library modules (backend, ply_io, utils). Also drop the dead docker/physics/simple.Dockerfile (its CMD referenced a removed script) and repoint the publish-sim-images workflow triggers and matrix at the new docker/carla/ locations. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to d3c0355: five files still pointed at the old alpasim_trafficsim/alpasim_physics.server locations and the old `physics_server` script name. Update them to the new carla_trafficsim_server / carla_physics_server module paths so the container images can actually import and launch after the move. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The CARLA extraction moved carla_trafficsim_server and carla_physics_server (plus entrypoint_physics.sh) into docker/carla/, but the repo-root .dockerignore uses an allowlist and never re-added those paths. Buildkit therefore reported the COPY targets as "not found" and both publish-sim-images matrix jobs failed on tier4 PR #8.
Both docker/carla/*_server/pyproject.toml declared [tool.setuptools.packages.find] with where=["."] and include=["<pkg>*"], but the .py files sit directly in the package directory (flat layout), not inside a nested <pkg>/ subdir. setuptools found nothing to include, so uv build produced wheels containing only dist-info metadata — physics.Dockerfile's post-install smoke test crashed with ModuleNotFoundError: No module named 'carla_physics_server'. Replace the find-based config with an explicit packages + package-dir mapping so the flat layout ships correctly.
The CARLA extraction (~15 GB unpacked in the carla-fetch stage) plus the CUDA runtime image and torch's build-isolation venv were exhausting the ~14 GB of headroom on / after the existing cleanup, producing "No space left on device" panics from the runner worker mid-build. `/mnt` on ubuntu-22.04 runners has ~70 GB free — bind Docker's data-root there before Buildx boots so all layers and cache land on the larger volume.
The CARLA extraction removed src/trafficsim/ (the CATK-based default traffic service) and repurposed the alpasim-trafficsim image tag for the CARLA-backed replacement. This left users without a default option — the wizard's trafficsim=carla profile was the only choice. Restore the CATK trafficsim package from tier4/main and reintroduce the monolithic Dockerfile that builds it, then split the publish matrix so both containers ship: - alpasim-trafficsim (default / CATK, built from ./Dockerfile) - alpasim-trafficsim-carla (CARLA-backed, docker/carla/trafficsim.Dockerfile) Wizard defines split to match: `trafficsim_image` still points at the default, and a new `carla_trafficsim_image` is threaded through the carla trafficsim profile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes NPC drift caused by CARLA's world clock never advancing, extracts the CARLA integration into a dedicated container tree, and adds a second CARLA-backed trafficsim variant alongside the default (CATK) one.
Clock/ownership fixes (commits
d70425df72f92d79fea1903689d413ac20a)Root cause (
d70425d): alpasim ran CARLA in synchronous mode but never calledworld.tick(), soworld.snapshot.timestamp.elapsed_secondswas frozen at zero. Trafficsim's snapshot writer stamped every NPC pose witht=0, and downstream consumers saw NPCs frozen in place.Hardening (
f72f92d): alpasim↔CARLA clock alignment — refuse misaligned targets, do not silently round drift.Ownership move (
79fea19/03689d4/13ac20a): world-clock ownership moves from trafficsim to physics. Physics is the one that actually models frame-to-frame world evolution, so it opens its own CARLA client and drivesworld.tick()as a side effect of the ego-facingground_intersectionRPC (via a newadvance_world_to_usfield). Trafficsim keeps its client for spawning + pose I/O but no longer touches world settings or ticks. CARLA host/port become fixed constants inside the physics container (no more configurable knob).CARLA extraction (
d3c0355bd7fc5b)src/trafficsim/was CARLA-specific after the CARLA integration commit. Move it out ofsrc/intodocker/carla/carla_trafficsim_server/, and mirror the same forsrc/physics/'s CARLA-only pieces →docker/carla/carla_physics_server/. This keepssrc/CARLA-free so non-CARLA callers can still import from it. The CARLA Dockerfiles live underdocker/carla/alongside the code they package.Restore default trafficsim (
df8255b)The extraction reused the
alpasim-trafficsimimage tag for the CARLA-backed variant, leaving users without a default option. Restore the CATK-basedsrc/trafficsim/package fromtier4/mainand reintroduce the monolithic./Dockerfilethat builds it. Publish matrix now ships both images so the wizard can pick between them:alpasim-trafficsim(default / CATK, built from./Dockerfile)alpasim-trafficsim-carla(CARLA-backed,docker/carla/trafficsim.Dockerfile)Wizard defines split accordingly:
trafficsim_imagestill points at the default, newcarla_trafficsim_imageis threaded through thetrafficsim=carlaprofile.CI infrastructure (
416e4730e3b8a25b87963).dockerignoreallowlist misseddocker/carla/**, causing buildkitnot founderrors at the COPY steps. Add it.carla_{physics,trafficsim}_serverpyproject.toml usedpackages.findwith awhere=["."]+include=["<pkg>*"]filter, but the .py files sit directly in the package directory (flat layout, no nested<pkg>/subdir). setuptools produced wheels containing only dist-info metadata →ModuleNotFoundErrorat the physics smoke test. Replace with an explicit[tool.setuptools]+package-dirmapping.ubuntu-22.04runners have ~14 GB free on/even afterjlumbroso/free-disk-space— not enough for CARLA (~15 GB unpacked) + CUDA image + torch build-isolation env. Relocate Docker's data-root to/mnt(~70 GB free) before Buildx starts.Design notes
apply_settingsandtick.PhysicsEvent(EGO)@50 → TrafficEvent@60 → PhysicsEvent(TRAFFIC)@70. Physics ticks CARLA on the ego-facing call, then trafficsim reads the freshly-ticked snapshot. NPCs see a one-frame lag on ego pose but avoid the drift-to-zero bug.Test plan
uv run pytest src/trafficsim/tests/— 16 passuv run pytest src/runtime/tests/— 420 pass (4 pre-existing unrelated failures)pre-commit run— all changed files passtrafficsim(CATK) +trafficsim-carla+physics-carlaall green🤖 Generated with Claude Code