Skip to content

fix: remove redundant autoconf from LCMService.start()#1515

Merged
spomichter merged 5 commits intodevfrom
summer/fix/lcmservice-autoconf-spam
Mar 11, 2026
Merged

fix: remove redundant autoconf from LCMService.start()#1515
spomichter merged 5 commits intodevfrom
summer/fix/lcmservice-autoconf-spam

Conversation

@SUMMERxYANG
Copy link
Contributor

@SUMMERxYANG SUMMERxYANG commented Mar 10, 2026

Problem

After PR #1503 (mac configurator) merged, users see repeated "Apply these changes now? [y/N]" prompts on startup — once per LCMService instance across all worker processes. The system configuration prompt loops indefinitely.

Solution

Remove the autoconf() call from LCMService.start(). The blueprint system already runs all system configurators via _run_configurators before deploying any modules to workers, making the per-service call redundant. Standalone scripts (e.g. drone.py) also call autoconf() explicitly before creating any services.

Also removes the now-dead LCMConfig.autoconf field and cleans up all callsites that passed it, since the field was only ever read by the deleted code path.

Breaking Changes

  • LCMConfig.autoconf field removed. Any code passing autoconf=True/False to LCMService, LCMPubSubBase, LCM, PickleLCM, etc. will get a TypeError. Fix: remove the autoconf= kwarg.

How to Test

  1. Run a blueprint that spawns multiple workers (e.g. G1 agentic blueprint)
  2. Verify the system configuration prompt appears only once on startup
  3. Verify LCM communication still works correctly

Contributor License Agreement

  • I have read and approved the CLA.

The blueprint system already runs configure_system via
_run_configurators before deploying any modules to workers.
The per-service call caused repeated prompts (once per LCMService
instance across all worker processes).
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 10, 2026

Greptile Summary

This PR fixes a regression introduced in #1503 where the autoconf() call inside LCMService.start() caused repeated interactive "Apply these changes now? [y/N]" prompts — once per LCM service instance across every worker process. The fix removes autoconf() from LCMService.start(), removes the now-dead LCMConfig.autoconf field, and cleans up all 14 call sites that were passing the autoconf= kwarg. The autoconf() function itself is preserved in lcmservice.py and re-exported via lcmpubsub.__all__, so standalone scripts (like drone.py) that call it explicitly before spinning up services continue to work correctly.

  • LCMConfig.autoconf field removed — a breaking change: any external code still passing autoconf=True/False to LCMService, LCMPubSubBase, LCM, PickleLCM, etc. will receive a TypeError.
  • All 14 in-tree call sites updated correctly.
  • Three test methods that validated the old autoconf= behavior are properly deleted.
  • docs/usage/transports/index.md:247 and docs/development/grid_testing.md:84 still show LCM(autoconf=True) in code snippets and were not updated — users following either document will hit an immediate TypeError.

Confidence Score: 4/5

  • Safe to merge — the logic change is correct and all in-tree code is updated, but two documentation files still reference the removed autoconf=True kwarg.
  • The core fix is clean and well-reasoned: removing a redundant per-instance autoconf() call that was causing repeated interactive prompts across worker processes. All 14 production and test call sites are correctly updated. The only gap is that two docs files (docs/usage/transports/index.md and docs/development/grid_testing.md) were not updated and still show the now-invalid LCM(autoconf=True) pattern, which will cause a TypeError for any user following those guides.
  • docs/usage/transports/index.md (line 247) and docs/development/grid_testing.md (line 84) still reference the removed autoconf=True kwarg.

Important Files Changed

Filename Overview
dimos/protocol/service/lcmservice.py Core fix: removes autoconf() call from LCMService.start() and drops the LCMConfig.autoconf field; clean, correct change that eliminates the redundant per-worker prompt.
dimos/protocol/service/test_lcmservice.py Tests properly updated: removes the three autoconf-specific test cases and the patch("autoconf") wrapper from all remaining service tests.
dimos/protocol/pubsub/impl/test_lcmpubsub.py Fixtures updated to drop autoconf=True from LCMPubSubBase, PickleLCM, and LCM constructors.
dimos/utils/cli/dtop.py Removes autoconf=True from PickleLCM constructor; the explanatory comment about TUI deadlocking is retained, which is still useful context.
dimos/utils/cli/lcmspy/run_lcmspy.py Drops autoconf=True from GraphLCMSpy; comment about TUI deadlock is preserved for context.
dimos/visualization/rerun/bridge.py Removes autoconf=True from all three LCM() instantiations in config defaults, docstring example, and run_bridge.
dimos/robot/unitree/g1/blueprints/primitive/uintree_g1_primitive_no_nav.py Removes autoconf=True from the G1 blueprint's rerun_config LCM instantiation.
dimos/robot/unitree/go2/blueprints/basic/unitree_go2_basic.py Removes autoconf=True from the Go2 blueprint's rerun_config LCM instantiation.

Sequence Diagram

sequenceDiagram
    participant BP as Blueprint / standalone script
    participant RC as _run_configurators
    participant LS as LCMService.start()
    participant AC as autoconf()

    Note over BP,AC: Before this PR (broken)
    BP->>RC: _run_configurators() — once
    RC->>AC: autoconf() ✅ prompt shown once
    BP->>LS: worker 1: service.start()
    LS->>AC: autoconf() ❌ prompt shown again
    BP->>LS: worker 2: service.start()
    LS->>AC: autoconf() ❌ prompt shown again

    Note over BP,AC: After this PR (fixed)
    BP->>RC: _run_configurators() — once
    RC->>AC: autoconf() ✅ prompt shown once
    BP->>LS: worker 1: service.start()
    Note right of LS: no autoconf() call
    BP->>LS: worker 2: service.start()
    Note right of LS: no autoconf() call
Loading

Comments Outside Diff (1)

  1. docs/usage/transports/index.md, line 247 (link)

    Stale autoconf=True kwarg in documentation

    This code snippet still uses LCM(autoconf=True), which is now removed as a breaking change in this PR. Any user following this documentation will get a TypeError: __init__() got an unexpected keyword argument 'autoconf' immediately.

    The same stale usage also appears in docs/development/grid_testing.md:84.

Last reviewed commit: d7d0580

SUMMERxYANG and others added 2 commits March 10, 2026 13:30
The blueprint system already runs configure_system via
_run_configurators before deploying any modules to workers.
The per-service call caused repeated "Apply these changes now?"
prompts (once per LCMService instance across all worker processes).

Also removes the now-dead LCMConfig.autoconf field and cleans up
all callsites that passed it.
@SUMMERxYANG
Copy link
Contributor Author

@greptile

SUMMERxYANG and others added 2 commits March 10, 2026 13:50
Standalone tools (rerun-bridge, dtop, lcmspy, topic echo) bypass the
blueprint system and no longer get autoconf() from LCMService.start().
Add check_only=True calls so they print warnings about missing system
config (multicast, sysctl buffers) without blocking on interactive
prompts.
@spomichter spomichter merged commit 98f2b92 into dev Mar 11, 2026
12 checks passed
@spomichter spomichter deleted the summer/fix/lcmservice-autoconf-spam branch March 11, 2026 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants