Skip to content

Fix System Config Values for LCM on MacOS and Refactor#1065

Merged
leshy merged 14 commits intodevfrom
jeff_lcmservice_cleanup
Jan 21, 2026
Merged

Fix System Config Values for LCM on MacOS and Refactor#1065
leshy merged 14 commits intodevfrom
jeff_lcmservice_cleanup

Conversation

@jeff-hykin
Copy link
Member

@jeff-hykin jeff-hykin commented Jan 20, 2026

Unit test with cd dimos/protocol/service;pytest, integration test with dimos --replay run unitree-go2

I needed to fix something in lcmservice, and Ivan had mentioned that the system checks in lcmservice were messy. I agreed so I refactored it

REFACTOR CHANGES:

  • system logic removed from lcmservce, put into system_configurator.py
  • LCM decides which system config values (Multicast, Socket Buffer Size), but that is it
  • Checking logic simplified

NEW STUFF (beyond refactor)

  1. Ask the user for permission instead of just attempting to change a bunch of system values (only asks once and lists everything that would be changed)
  2. The missing ulimit -n check for MacOS (default max number of open files is 256 on MacOS) was added.
  3. Turns out the max buffer size on macos is in fact 8Mb (8_388_608). Me seeing that 67_108_864 (64Mb) "worked" was just the setter command failing with a warning and not being very loud (while the rest of dimensional still ran). It seems that 8_388_608 is a hardcoded limit in the MacOS kernel. The logic now is split where MacOS is using the 8_388_608 value, and Linux uses 67_108_864

NOTE:

  • test_high_volume_messages seems flakey on CI @leshy (on dev or this PR). Passes every time for me on a dimensional laptop. On my mac its not even close to passing (450's out of 1000)

@jeff-hykin jeff-hykin requested a review from a team January 20, 2026 03:50
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 20, 2026

Greptile Overview

Greptile Summary

Refactors system configuration logic out of lcmservice.py into new system_configurator.py with better separation of concerns. Adds user permission prompts before modifying system settings and implements missing MacOS checks.

Key improvements:

  • Extracts system config checks into reusable SystemConfigurator base class with platform-specific implementations
  • Adds user consent prompt before applying system changes (lists all changes upfront)
  • Fixes MacOS multicast route command (was incorrectly using ip route add, now uses route add)
  • Adds missing MacOS ulimit check for file descriptors (default is 256, increased to 65536)
  • Corrects MacOS buffer size limit to 8MB (kernel hardcoded max) instead of attempting 64MB

Minor issue:

  • Line 337 uses MAX_POSSIBLE_RECVSPACE instead of TARGET_RECVSPACE (inconsistent with class variable naming)

Confidence Score: 4/5

  • Safe to merge after fixing the variable naming inconsistency on line 337
  • Well-structured refactor with comprehensive test coverage, but contains one naming inconsistency that could cause confusion
  • dimos/protocol/service/system_configurator.py requires a one-line fix for variable naming consistency

Important Files Changed

Filename Overview
dimos/protocol/service/system_configurator.py New file extracting system configuration logic from LCMService; adds user permission prompts, MacOS ulimit check, and proper buffer size limits per platform
dimos/protocol/service/lcmservice.py Refactored to delegate system configuration to SystemConfigurator classes; removed 200+ lines of system check logic

Sequence Diagram

sequenceDiagram
    participant User
    participant LCMService
    participant configure_system_for_lcm
    participant SystemConfigurator
    participant OS as Operating System

    User->>LCMService: start()
    LCMService->>configure_system_for_lcm: configure_system_for_lcm(check_only=False)
    
    alt Platform is Linux
        configure_system_for_lcm->>configure_system_for_lcm: Create LinuxMulticast & LinuxBuffer
    else Platform is Darwin (macOS)
        configure_system_for_lcm->>configure_system_for_lcm: Create MacOSMulticast, MacOSBuffer & MacOSUlimit
    end
    
    configure_system_for_lcm->>SystemConfigurator: configure_system(checks)
    
    alt CI Environment
        SystemConfigurator-->>configure_system_for_lcm: Skip (CI detected)
    else Not CI
        loop For each check
            SystemConfigurator->>SystemConfigurator: check()
            SystemConfigurator->>OS: Read system values (sysctl, netstat, etc.)
            OS-->>SystemConfigurator: Current values
        end
        
        alt All checks pass
            SystemConfigurator-->>configure_system_for_lcm: No changes needed
        else Some checks fail
            SystemConfigurator->>User: Display required changes & prompt
            User-->>SystemConfigurator: User response (y/n)
            
            alt User confirms
                loop For each failing check
                    SystemConfigurator->>SystemConfigurator: fix()
                    SystemConfigurator->>OS: Apply changes (sudo if needed)
                    OS-->>SystemConfigurator: Success/Failure
                end
            else User declines & critical check failed
                SystemConfigurator->>SystemConfigurator: raise SystemExit(1)
            end
        end
    end
    
    configure_system_for_lcm-->>LCMService: Configuration complete
    LCMService->>LCMService: Start LCM loop
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

# On macOS, use multicast with TTL=0 to keep traffic local
self.url = _DEFAULT_LCM_URL_MACOS
if self.url is None:
self.url = _DEFAULT_LCM_URL
Copy link
Member Author

@jeff-hykin jeff-hykin Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this confirmed to be the default LCM for all OS's. I wanted to get it from lcm directly but it seems they don't expose it

# ----------------------------- generic enforcement of system configs -----------------------------


def configure_system(checks: list[SystemConfigurator], check_only: bool = False) -> None:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is generic enough that it maybe shouldn't live under protocol/service.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, can move later when other systems need it

@leshy
Copy link
Contributor

leshy commented Jan 20, 2026

test_high_volume_messages seems flakey on CI @leshy (on dev or this PR). Passes every time for me on a dimensional laptop. On my mac its not even close to passing (450's out of 1000)

This test sometimes in CI registers 1001 msg, likely due to some parallel test exec on the same machine (but never less) so something could be off on your end? lcm handles 50k messages per sec easily on my machine

2026-01-20_22-57

you can run
python -m pytest -svm tool -k lcm dimos/protocol/pubsub/benchmark/test_benchmark.py

to check on your end, curious about mac and if it can be tuned.

I assume it's mac network stack but to make sure you can run raw udp test and see what's the lcm encode/decode slowdown cost
python -m pytest -svm tool -k udp dimos/protocol/pubsub/benchmark/test_benchmark.py

here I get 250k msgs / sec

2026-01-20_23-00

@leshy
Copy link
Contributor

leshy commented Jan 20, 2026

Ask the user for permission instead of just attempting to change a bunch of system values (only asks once and lists everything that would be changed)

yeah this is good idea, will review soon tnx

Copy link
Contributor

@leshy leshy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great, can push in, leaving comments on benchmarking for macos people

@leshy leshy merged commit e19e43c into dev Jan 21, 2026
13 checks passed
spomichter added a commit that referenced this pull request Jan 23, 2026
… Unitree Go2 Navigation & Exploration Beta

Pre-Release v0.0.8: Unitree Go2 Navigation & Exploration Beta, Transport Updates, Documentation updates, Rerun fixes, Person follow, Readme updates

## What's Changed
* Small docs clarification about stream getters by @leshy in #1043
* Fix split view on wide monitors by @jeff-hykin in #1048
* Docs: Install & Develop  by @jeff-hykin in #1022
* Add uv to nix and fix resulting problems by @jeff-hykin in #1021
* v0.0.8 by @paul-nechifor in #1050
* Style changes in docs by @paul-nechifor in #1051
* Revert "Add uv to nix and fix resulting problems" by @leshy in #1053
* Transport benchmarks + Raw ros transport by @leshy in #1038
* feat: default to rerun-web and auto-open browser on startup (browser … by @Nabla7 in #1019
* bbox detections visual check by @leshy in #1017
* fix: only auto-open browser for rerun-web viewer backend by @Nabla7 in #1066
* move slow tests to integration by @paul-nechifor in #1063
* Streamline transport start/stop methods by @Kaweees in #1062
* Person follow skill with EdgeTAM by @paul-nechifor in #1042
* fix: increase costmap floor z_offset to avoid z-fighting by @Nabla7 in #1073
* Fixed issue #1074 by @alexlin2 in #1075
* ROS transports initial by @leshy in #1057
* Fix System Config Values for LCM on MacOS and Refactor by @jeff-hykin in #1065
* SHM Transport basic fixes by @leshy in #1041
* commented out Mem Transport test case by @leshy in #1077
* Docs/advanced streams update 2 by @leshy in #1078
* Fix more tests by @paul-nechifor in #1071
* feat: navigation docker updates from bona_local_dev by @baishibona in #1081
* Fix missing dependencies by @Kaweees in #1085
* Release readme fixes by @spomichter in #1076

## New Contributors
* @baishibona made their first contribution in #1081

**Full Changelog**: v0.0.7...v0.0.8
@jeff-hykin jeff-hykin deleted the jeff_lcmservice_cleanup branch January 24, 2026 05:00
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