[Convenience PR / branch: all of the docs / all of YM's changes] #809
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Greptile OverviewGreptile SummaryThis PR implements a comprehensive documentation overhaul for the DimensionalOS robotics framework, establishing a complete MkDocs-based documentation system with tutorials, API references, and conceptual guides. The changes add extensive docstrings throughout the codebase using Important Files Changed
Confidence score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (3)
-
dimos/core/module_coordinator.py, line 58 (link)logic: Class-level mutable default creates shared state across instances. Should use
self._deployed_modules = {}in__init__ -
dimos/core/transport.py, line 80 (link)logic: Duplicate TypeVar declaration shadows the earlier one on line 65
-
dimos/protocol/skill/coordinator.py, line 826-828 (link)style: Debug print statements left in production code - consider removing before release
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
47 files reviewed, 10 comments
ym-han
force-pushed
the
all-ym-changes
branch
from
74c6179 to
30fd0ed
Compare
ym-han
force-pushed
the
all-ym-changes
branch
3 times, most recently
from
f89578a to
e45ab00
Compare
docs/concepts/skills.md
Outdated
|
|
||
| 1. **Becomes an agent-callable tool** - The decorator generates an OpenAI-compatible function schema from the method signature and docstring | ||
| 2. **Executes in background threads** - Skills run concurrently without blocking the agent | ||
| 3. **Reports state via messages** - Each execution tracks state (pending → running → completed/error) |
There was a problem hiding this comment.
this is an internal detail of messaging protocol related to skills, there are many details to this protocol, and users need not understand it so I'm not sure why are we explaining this here?
this is also an incomplete explanation, actual states are
pending = 0
start = 1
stream = 2
reduced_stream = 3
ret = 4
error = 5
There was a problem hiding this comment.
Good catch, this needs to be refactored, yeah.
I think this was meant to be SkillStateEnum, which, as of the time of this PR, only had
pending = 0
running = 1
completed = 2
error = 3
(I.e., this isn't factually wrong if it's meant to be SkillStateEnum.)
I don't remember this part very well now, but I think I was tempted to leave that there because it might be helpful for understanding skillspy's output. But you are right in that it either needs more explanation or should just be removed. Maybe the thing to do is to only bring up SkillStateEnum when actually explaining Skillspy -- will remove that third point, unless you disagree.
docs/concepts/skills.md
Outdated
| At a high level, skills are wrappers over lower-level robot capabilities. But at a more prosaic level, a skill is just a method on a Module decorated with `@skill` that: | ||
|
|
||
| 1. **Becomes an agent-callable tool** - The decorator generates an OpenAI-compatible function schema from the method signature and docstring | ||
| 2. **Executes in background threads** - Skills run concurrently without blocking the agent |
There was a problem hiding this comment.
it executes on a hosting module which is a separate process, I'm not sure thread is meant abstractly here, but this is an important detail to be clear about
There was a problem hiding this comment.
Yes, this is misleading in a bad way. Am changing this to "Executes asynchronously - Skills run without blocking the agent"
docs/concepts/skills.md
Outdated
|
|
||
| At a high level, skills are wrappers over lower-level robot capabilities. But at a more prosaic level, a skill is just a method on a Module decorated with `@skill` that: | ||
|
|
||
| 1. **Becomes an agent-callable tool** - The decorator generates an OpenAI-compatible function schema from the method signature and docstring |
There was a problem hiding this comment.
tool call definition encoding can be dependant on the model we are integrating, I'm not sure if this is openai every time? langchain controls this layer, I honestly don't know, and I'm not sure why we are explaining this to a user at this point
There was a problem hiding this comment.
Changing this to "Becomes an agent-callable tool - The decorator generates a tool schema from the method signature and docstring"
| # The following dunder methods are for Dask serialization: | ||
| # Module instances are serialized when deployed to worker processes. | ||
| # We return {} in __getstate__ since this class has no custom state to preserve. | ||
| def __getstate__(self): |
There was a problem hiding this comment.
I can't remb well now, but I'm guessing I added this to explain how to write a skill 'from scratch', without the helper SkillModule. (The SkillModule had this, and the blueprint readme also had it.) But it might be more confusing than helpful
docs/concepts/modules.md
Outdated
|
|
||
| <!-- Citation: module.py:113-126 - _close_module() shows independent resource management (loop, rpc, tf, disposables) --> | ||
|
|
||
| **Distributed execution** - Modules run as Dask actors across a cluster. Deploy computationally intensive perception on GPU workers while running navigation on CPU workers. The system handles network communication, serialization, and RPC automatically. |
There was a problem hiding this comment.
GPU workers vs CPU workers? there is no such thing in dimos
There was a problem hiding this comment.
Yeah. In principle Modules should help with this, but yes, it's too misleading in this context. Will remove that sentence.
| <!-- Citation: perception/spatial_perception.py:64 - Real SpatialMemory module with color_image: In[Image] declaration --> | ||
|
|
||
| > [!NOTE] | ||
| > The core takeaway: modules can operate on any underlying transport. The same module code deploys across local shared memory, network protocols, or distributed clusters by changing just the transport configuration. |
There was a problem hiding this comment.
a lot of this is theoretical, like ok, how do I run on a distributed cluster? We have no code for this, we don't want to claim things like this that don't exist yet
There was a problem hiding this comment.
Yeah you're right, I had let this slide because I was thinking too much about what this in principle allows for.
| **Monitor long-running skills** - Use `skillspy` to watch skill execution in real-time. Skills are tracked in an execution database showing what's currently running and what has completed—invaluable for debugging navigation or other long operations. See the [skill basics tutorial](../tutorials/skill_basics/tutorial.md) for an example of this. | ||
|
|
||
| > [!WARNING] | ||
| > **Don't use both `@skill` and `@rpc` decorators on a single method** - The `@skill` wrapper can't be pickled for LCM transport. Use `@skill()` for agent tools, `@rpc` for module-to-module calls. |
There was a problem hiding this comment.
I think I got an error when literally trying to do something like
# but can't remb the exact order I had tried this in
@skill
@rpc
def method():
docs/concepts/skills.md
Outdated
|
|
||
| We've seen how skills can be made available to agents as tools they can call. Often, however, we don't just want agents making tool calls -- we also want to relay updates from the tool calls, from the skills, back to the agent. | ||
|
|
||
| Some of this behavior already comes as a default: if you decorate the method with `@skill()`, the agent will be notified with the *return value* of the method when the skill finishes (because the default value of the `ret` parameter is `Return.call_agent`). |
There was a problem hiding this comment.
ret parameter? what is that? why are we talking about this? this is the first mention of this or Return.call_agent
There was a problem hiding this comment.
Good catch, I've removed that parenthetical comment.
|
|
||
| ## See also | ||
|
|
||
| - [The Skills API reference](../api/skills.md) |
There was a problem hiding this comment.
I feel like after this I don't understand the concepts of reducers, active/passive streams etc, idk about tooling (skillspy, agentspy, humancli) idk if this is explained elsewhere, will go through tutorials
ym-han
force-pushed
the
all-ym-changes
branch
from
e45ab00 to
c8a3288
Compare
spomichter
force-pushed
the
all-ym-changes
branch
from
c8a3288 to
7fd13bc
Compare
|
Too many files changed for review. |
1 similar comment
|
Too many files changed for review. |
spomichter
force-pushed
the
all-ym-changes
branch
from
7fd13bc to
cd6b51d
Compare
|
Too many files changed for review. |
1 similar comment
|
Too many files changed for review. |
spomichter
force-pushed
the
all-ym-changes
branch
from
32609ee to
6cf16f0
Compare
|
Too many files changed for review. |
1 similar comment
|
Too many files changed for review. |
|
Too many files changed for review. |
Re-opening new PR b/c couldn't open old one. (leshy had also left some comments on the old PR: #787)
To get a quick sense for what docs have been done and what else is left, see #736 (comment) (please do read this comment first).
How to see the docs
First, make sure you've installed the docs deps:
Local Development Server
To start a local server with hot reload:
mkdocs serve # Then open <http://127.0.0.1:8000/> in your browser.Build Static Site
To build the static documentation site:
The output (which includes the various
llm.txes) will be in thesite/directory.See also the note in docs/development.md on marimo notebooks.
Notes on doc tech stack choices
Misc resources
Doc site packages
Ivan was asking, so: for reference, the non-Mintlify docs sites solutions I'm aware of include:
Unfortunately, I don't know which of these would be most suitable for you. But the Zensical stuff does suggest that mkdocs isn't a good long-term solution (I went with mkdocs initially just because it's a common choice; hadn't known then that it didn't seem well-maintaned).