Conversation
slecleach
left a comment
There was a problem hiding this comment.
Thanks for the Sumo backend work — I walked through the diff in detail and found two behavioral regressions to fix before merge:
SimulationNode._init_simcan now fail for cfg-registered custom tasks
- In
judo/app/dora/simulation.py,_init_sim()callsget_registered_tasks().get(task_name)before any backend instance is constructed. - Custom tasks are registered in
Simulation.__init__viaregister_tasks_from_cfg(task_registration_cfg), which only runs whensim_backend_cls(...)is instantiated. - That means
init_taskvalues supplied only throughtask_registration_cfgnow raiseValueErrorduring node startup.
- Switching task resets controller state/config unexpectedly
- In
judo/app/dora/controller.py,update_task()now rebuilds the controller viaself._make_controller_fn(init_task=..., init_optimizer=...). - This path does not preserve runtime state/config that previously survived task switches (notably controller config overrides like horizon/control_freq), since
make_controller()creates a freshControllerConfig. - It also omits the node's original registration/customization args (
task_registration_cfg,optimizer_registration_cfg, and any custom kwargs), so behavior can drift after a task change.
Please preserve existing controller/task/optimizer config where intended and ensure task registration from cfg occurs before task lookup in simulation init.
I see the concern in point 1, but we never supply the For part 2, we don't mind the overrides being overwritten due selecting a new task. We also only ever reference the optimizer as one source of truth due to a change I made in the API, meaning that any new controller automatically references its optimizer config as the sole source of truth. The behaviour can drift if the configs are changed, but the configs aren't meant to change in between runs. This could be an issue for |
Alterations which enable support for Sumo and other C++ backend rollouts.