Repository navigation
runtime: make route generators plugin-selectable - #179
amtellezfernandez wants to merge 5 commits into
Conversation
| route_start_offset_m: float = 0.0 | ||
| # Optional alpasim.route_generators entry point; overrides route_generator_type, | ||
| # including NONE. Unset preserves built-in routing without alpasim_plugins. | ||
| route_generator_plugin: str | None = None |
There was a problem hiding this comment.
since we don't have to worry about backwards compatibility, I would try to bring this somehow into the route_generator_type field. In this case, we would need to drop the enum and move to a string, but I think that would maybe be a more natural approach.
The main issue I have is that we have a parallel path for configuration and it becomes a little awkward reading and maintaining
There was a problem hiding this comment.
Sorry for the slow reply @mwatson-nvidia
Done in 2146d2c: route_generator_type is now a string, MAP/RECORDED/NONE keep their meaning, any other value resolves through alpasim.route_generators, and both the RouteGeneratorType enum and route_generator_plugin are gone.
One thing the enum gave us for free was OmegaConf rejecting an unknown value at config load. A plain string would push a typo to EventBasedRollout, failing every rollout after the services are up, so parse_simulator_config now resolves the name at startup, next to validate_renderer_config.
Along the way I also fixed a misleading "not found" error in PluginRegistry for plugins that fail to import (be78f3a). It's shared by every plugin group, so I can split it out if you'd prefer. Details and test results are in the updated description.
Let me know if this matches what you had in mind.
Drop the RouteGeneratorType enum and the parallel route_generator_plugin field. route_generator_type is now a string: MAP, RECORDED and NONE keep their built-in meaning, and any other name resolves to an alpasim.route_generators entry point. The enum rejected an unknown value when the config was parsed. To keep that, parse_simulator_config resolves a plugin name at startup, so a typo fails before services start rather than once per rollout. Built-in routing still does not import alpasim_plugins; an unknown name without it installed raises a ValueError instead of an ImportError.
PluginRegistry swallowed load exceptions with a warning, so requesting a plugin whose module raised on import produced "not found ... Available: []", pointing at installation rather than at the import error. Record the failure and raise PluginNotFoundError with the original exception.
Closes #150
Route generators become selectable through the plugin system, via the existing
route_generator_typefield. TodayRouteGenerator.create()dispatches over a closed enum, so a custom route source (an external routing service, hand-authored scenario routes) means editing core code.Design
SimulationConfig.route_generator_typeis now astr, default"MAP":MAP,RECORDEDandNONEkeep their existing meaning, and still don't importalpasim_plugins.alpasim.route_generatorsentry-point group. The registered class providesfrom_context(recorded_waypoints_in_local, vector_map, *, route_start_offset_m=0.0)and returns aRouteGenerator;vector_mapcan beNone.mapwas rejected before and still is.The
RouteGeneratorTypeenum is removed, along with the separateroute_generator_pluginoverride field this PR originally proposed (and #150 sketched). Per the review, there is now one configuration path; the inline comment onconfig.pyrefers to the earlier diff.Dropping the enum also dropped a check it did for free: OmegaConf rejected an unknown value when the config was parsed. With a plain string, a typo would only surface in
EventBasedRollout, once per rollout and after the services had started;worker/main.pylogs each one asRollout FAILEDand the run carries on. To keep the old behaviour,parse_simulator_confignow callsvalidate_route_generator_confignext tovalidate_renderer_config, so an unknown name raisesPluginNotFoundErrorat startup. The check and the factory shareresolve_route_generator_plugin(), so they can't disagree about what is valid. Withoutalpasim_pluginsinstalled, a non-built-in name raises aValueErrorthat says so, instead of anImportError.Two smaller changes:
MAP,RECORDEDorNONEcan never be selected, since the built-ins are matched first. Startup logs a warning naming it. It's a warning rather than an error so that an installed package with a bad name doesn't break runs that only use built-ins.PluginRegistry.get_available()catches load exceptions and only logs them, so requesting a plugin whose module fails to import currently raisesPlugin 'x' not found ... Available: [], which points at installation rather than at the import error. The registry now records the failure, andget()raisesPluginNotFoundError: Plugin 'x' in <group> is installed but failed to load: ModuleNotFoundError: .... This touches the registry shared by every group, so it's its own commit (be78f3a); happy to split it into a separate PR if you'd rather review it on its own.Migration: existing YAML (
route_generator_type: MAP/RECORDED/NONE, including the twoNONEpresets undersrc/wizard/configs/exp/presets/) parses unchanged. Python code importingRouteGeneratorTypepasses the string instead, e.g."RECORDED"forRouteGeneratorType.RECORDED.Out of scope on purpose: migrating the built-in generators to entry points, and giving plugins scene identity, the rollout seed or plugin-specific configuration.
from_contextreceives the same inputs the built-ins use; what else a real plugin needs should come from a concrete use case.Tests
src/runtime/tests/test_route_generator.py(20 passed): a plugin selected by name, with and without a map; an unknown name raising instead of falling back; and, in a fresh interpreter withalpasim_pluginsblocked, built-in routing working and a plugin name raising theValueError.src/runtime/tests/test_validation.py(6 new): the three built-ins pass, a registered plugin passes, a typo (map) fails at startup, and a reserved-name plugin logs the warning.src/plugins/tests/test_plugins.py: an entry point pointing at a missing module is reported as a load failure.Full runtime suite: 522 passed (the one deselected test is the
manualmarker fromaddopts), includingtest_route_generator_mapandtest_route_generator_map_sanity_off_routeon their Git LFS fixtures. Plugins: 9 passed;test_models_registry_has_core_models_when_driver_installedandtest_info_main_includes_models_when_driver_installedfail becausealpasim.modelsis empty in my environment, and fail identically onmain(affc2ea). black 24.10.0, isort 5.13.2 and flake8 7.1.1 pass at the pre-commit settings. Run with protobuf bindings regenerated from this branch'ssrc/grpc.Downstream, AlpaBridge drives a test-only route plugin through the real
UnboundRollout,EventBasedRolloutandPolicyEvent, over gRPC to its driver service; all four cases (headings 0° and 90°, start offsets 0 and 8 m) pass against this branch (test).Happy to adjust the shape if you'd draw any of it differently.