-
Notifications
You must be signed in to change notification settings - Fork 2.7k
refactor all information sources (including ipless rdma discovery) #928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c5fd38b to
b48fc01
Compare
rltakashige
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to re-review this not at 3am, but it looks good to me on first pass. I've added some minor comments.
| class SocketConnection(FrozenModel): | ||
| sink_multiaddr: Multiaddr | ||
|
|
||
| def __hash__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does SocketConnection need a hash where RDMAConnection does not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Multiaddr type is unhashable as-is, where the pair of strings is hashable. I could fix this instead by making Multiaddr inherit from FrozenModel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rather that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! the port should not be part of the hash - two socketconnections with the same port are equivalent data. That does mean I should just store the ipaddress here, but we don't have a dedicated ipaddress type on this branch and we just mock multiaddrs everywhere instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gm btw haha
src/exo/shared/apply.py
Outdated
| profile.chip_id = info.chip | ||
| # TODO: makes me slightly sad | ||
| case Sequence(): | ||
| if info != []: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if event == []: break, just to reduce even one indent here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't break out of a match. I just realised that there are some legit meanings of [] though, so I can't merge this until I've split up these events properly. no network interfaces is possible and is distinct from no TBConnections, and both should be applied to state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.
f3e8ab2 to
c86c5fa
Compare
c0b532a to
ed37aec
Compare
e56de1a to
b5319d6
Compare
ec02c75 to
dfd866a
Compare
Dashboard fixes (TypeScript errors from `npm run check`): - TopologyGraph.svelte: remove reference to deleted sendBackMultiaddr property, fix type inference for debug edge labels - ModelCard.svelte: add missing topoWidth/topoHeight to early return - +page.svelte: fix nested property access for deviceRank Backend fix: - info_gatherer.py: send initial MiscData on startup so friendly name appears immediately instead of showing "Unknown" until it changes Co-Authored-By: Claude Opus 4.5 <[email protected]>
The API changed topology.connections from an array to a nested mapping:
{ source: { sink: [SocketConnection | RDMAConnection] } }
- Update type definitions for RawSocketConnection and RawRDMAConnection
- Update transformTopology to iterate over nested mapping structure
- Handle snake_case ip_address from Multiaddr computed fields
Co-Authored-By: Claude Opus 4.5 <[email protected]>
bc6a310 to
7e333bc
Compare
JakeHillion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Motivation
Information gathering is tightly coupled to MacMon - we should start generalizing our information sources so we can add more in future.
Changes
Added a new system to gather any information. Currently, it is attached to the Worker - though this is mostly to keep the data processing logic simple. It could be made independent quite easily.
I also refactored topology to include different kinds of connections as we can gather RDMA connections without having a pre-existing socket connection, and made the relevant placement updates. We should no longer need the network locations script in the app.
Other sources of information now include:
Limitations
The current events added by the InfoGatherer are much too broad and don't follow proper Pydantic validation.
Model and Chip are not cross platform concepts.
We do not differentiate between unified and non-unified memory systems. this should be added to static information ASAP.
A lot of this data collection is based on simple timers. Watching the SC store on macos is the correct way to gather some of this information, but requires a detour into rust for macos.
Why It Works
The InfoGatherer is a generic subsystem which returns a union of metric datatypes. It writes them to an event, which is applied to state. It is currently re-spawned with the worker so each cluster receives the correct information.
As for topology, macOS identifies TB ports with a uuid in SPThunderboltDataType, and also stores remote uuids if it can find them. These changes read that data with the system_profiler, hopefully not so often as to cause notable performance impacts (though this should be tuned) but frequently enough for moderate responsiveness.
As we can identify TB connections between devices without needing ips attached to each interface, we can remove the network setup script (almost) completely.
Test Plan
Manual Testing
Spawn RDMA instances without enabling DHCP on the RDMA interfaces.
Automated Testing
Updated the current master and shared tests to cover the topology refactor and new events.