Skip to content

[Tiny] use ip instead of ifconfig#976

Merged
spomichter merged 4 commits intodevfrom
jeff_fix_ifconfig
Jan 13, 2026
Merged

[Tiny] use ip instead of ifconfig#976
spomichter merged 4 commits intodevfrom
jeff_fix_ifconfig

Conversation

@jeff-hykin
Copy link
Member

  • super small change, test with any dimos run command
  • ifconfig is deprecated, not installed on default ubuntu, and we already use the ip command which can do what we want
  • tests updated

(this matters for day-1 install instructions)

@jeff-hykin jeff-hykin requested a review from a team January 12, 2026 02:54
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 12, 2026

Greptile Overview

Greptile Summary

⚠️ Critical Issue: PR Metadata Mismatch

The PR title and description are completely incorrect. The PR claims to be "use ip instead of ifconfig" for day-1 install instructions, but the actual changes are entirely unrelated:

What This PR Actually Does:

  1. Fixes MuJoCo RGB/BGR color channel bug - Adds format=ImageFormat.RGB when converting MuJoCo frames to Image objects (fixes red/blue channel swap in visualization)
  2. Improves mujoco_menagerie initialization - Changes from generic import mujoco_playground to explicitly calling mjx_env.ensure_menagerie_exists() to prevent timeout issues
  3. Code cleanup - Removes unused imports (SystemMessage, threading, Any)
  4. Restores uv.lock - Adds back the dependency lock file for reproducible builds
  5. Typo fix - Corrects "menajerie" to "menagerie" in comment

What This PR Does NOT Do:

  • Does not modify any ifconfig commands
  • Does not change any ip commands
  • Does not affect day-1 install instructions

The actual ifconfig usage exists in dimos/protocol/service/lcmservice.py and its tests, which are not modified by this PR.

Technical Assessment:

The code changes themselves are good bug fixes and cleanups. The MuJoCo RGB/BGR fix addresses a real visualization bug. However, the PR metadata is misleading and will cause confusion in the project history.

Confidence Score: 3/5

  • Code changes are safe and beneficial, but PR metadata is completely incorrect
  • Score of 3 reflects that while the actual code changes are good (bug fixes and cleanup that would rate 4-5), the severe PR metadata mismatch is a significant issue. The PR title "use ip instead of ifconfig" and description about day-1 install instructions are completely unrelated to the actual changes (MuJoCo fixes, import cleanup, uv.lock restoration). This will cause confusion in git history, make the PR impossible to find when searching for either ifconfig changes OR MuJoCo fixes, and suggests possible branch/commit mix-up.
  • The code changes are solid. Main concern is the PR metadata mismatch - verify this is the intended PR and update title/description accordingly.

Important Files Changed

File Analysis

Filename Score Overview
dimos/agents/vlm_agent.py 5/5 Removed unused SystemMessage import - safe code cleanup with no functional changes
dimos/protocol/mcp/test_mcp_module.py 5/5 Removed unused threading import - safe code cleanup with no functional changes
dimos/robot/unitree_webrtc/mujoco_connection.py 4/5 Fixed RGB/BGR color channel swap bug, corrected typo, and improved mujoco_menagerie initialization. Good bug fixes, but depends on ensure_menagerie_exists() method existing in mujoco_playground package.
uv.lock 4/5 Restored lock file for reproducible builds (10,066 lines added)

Sequence Diagram

sequenceDiagram
    participant MC as MujocoConnection
    participant MP as mujoco_playground
    participant MR as MuJoCo Renderer
    participant IMG as Image.from_numpy()
    participant VIZ as Rerun Visualization

    Note over MC,MP: Initialization Phase
    MC->>MP: Import mjx_env module
    MC->>MP: Call ensure_menagerie_exists()
    MP-->>MC: Menagerie data downloaded
    
    Note over MC,MR: Video Frame Processing
    MC->>MR: Request video frame
    MR-->>MC: RGB uint8 numpy array
    
    Note over MC,IMG: Before Fix (Bug)
    MC->>IMG: Image.from_numpy(frame)
    Note right of IMG: Defaults to BGR format
    IMG-->>MC: Image with swapped channels
    MC->>VIZ: Publish image
    Note right of VIZ: Red/Blue swapped!
    
    Note over MC,IMG: After Fix (Correct)
    MC->>IMG: Image.from_numpy(frame, format=ImageFormat.RGB)
    Note right of IMG: Explicit RGB format
    IMG-->>MC: Image with correct channels
    MC->>VIZ: Publish image
    Note right of VIZ: Colors display correctly
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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@leshy
Copy link
Contributor

leshy commented Jan 12, 2026

This should go in dev not main right? diff looks weird

@jeff-hykin jeff-hykin changed the title use ip instead of ifconfig [Tiny] use ip instead of ifconfig Jan 12, 2026
@jeff-hykin jeff-hykin changed the base branch from main to dev January 12, 2026 03:44
@spomichter
Copy link
Contributor

@jeff-hykin I assume you're running ubuntu 2404. if ifconfig works on 2204. so this change would break 2204 pretty sure

Copy link
Contributor

@spomichter spomichter left a comment

Choose a reason for hiding this comment

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

need to test on 2204

@leshy
Copy link
Contributor

leshy commented Jan 12, 2026

@jeff-hykin I assume you're running ubuntu 2404. if ifconfig works on 2204. so this change would break 2204 pretty sure

pretty sure this is ok, ifconfig is actually super old and an extra dependancy, idk if ubuntu comes with it by default, everyone switched to ip

@spomichter spomichter merged commit 29b482b into dev Jan 13, 2026
13 checks passed
@spomichter spomichter deleted the jeff_fix_ifconfig branch January 13, 2026 05:04
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.

3 participants