Skip to content

Add agents file.#173

Open
syamajala wants to merge 2 commits intomasterfrom
agents
Open

Add agents file.#173
syamajala wants to merge 2 commits intomasterfrom
agents

Conversation

@syamajala
Copy link
Contributor

Try to make opencode work better for ami by adding an agents.md.

@syamajala syamajala requested a review from carbonscott March 11, 2026 11:43
Copy link

@carbonscott carbonscott left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! Having an architecture guide for AI agents is a great idea. The overall structure and descriptions look solid.

I did a cross-check against the codebase and noticed a few things that might be worth double-checking:

Potential inaccuracies

  1. graphkit vs networkfox (Execution Graph section) — ami/graphkit_wrapper.py appears to import from networkfox, not graphkit. The file is named graphkit_wrapper.py but the actual library seems to be networkfox. Is this intentional/historical?

  2. Manager inheritance — The doc describes Manager similarly to other components that extend Node, but Manager seems to only extend Collector (not Node). Worth clarifying?

  3. ZMQ socket types — The doc says PUB/SUB, but the code appears to use XPUB/SUB for manager→workers and manager→clients. There also seems to be a ROUTER/DEALER pattern for view data that isn't mentioned.

  4. ami-mpi description — The doc says ami-mpi -n 4 psana://... for multiple workers with psana, but ami-mpi looks like a bash wrapper script (not a Python entry point) with a 1-node limit. Is ami-remote the actual underlying entry point here?

  5. "ami-local does not support multiple workers with psana" — I couldn't find this restriction in the code (-n seems to accept any value with any source). Is this a practical limitation rather than a code-enforced one?

  6. AutoExport — Listed as a class in the Communication section, but it looks like an AutoName instance (AutoExport = AutoName("_export_")). Minor, but could confuse an AI agent.

Minor suggestions

  • The GUI is PyQtGraph-based (via pyqtgraph and qtpy), not raw PyQt — might be worth noting
  • The ami-worker example might need a -H manager_host flag to be complete
  • The file tree is a useful subset but missing some notable files (local.py, remote.py, sync.py, forkedpdb.py, console.py, monitor.py)

Overall this is a really useful doc — just flagging these in case any of them are actual errors vs. things I'm misreading. You'd know better than me!

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com


### Starting AMI

1. **Workers**: `ami-worker -n 3 psana://exp=rix101331225,run=156`

Choose a reason for hiding this comment

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

In distributed mode, ami-worker also requires -H manager_host to connect to the manager. Might be worth including for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we normally just run everything on the same host, but if we go to multi-node then it would be needed.

Choose a reason for hiding this comment

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

Could we add a note somewhere to reflect "we normally just run everything on the same host"?

4. **Local mode**:
- Random source: `ami-local -n 3 random://examples/worker.json`
- Psana offline run (single worker): `ami-local -f interval=1 -b 1 psana://exp=rix101331225,run=156`
- Psana with multiple workers: `ami-mpi -n 4 psana://exp=rix101345725,run=67` (ami-local does not support multiple workers with psana)

Choose a reason for hiding this comment

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

Two things here: (1) ami-mpi appears to be a bash wrapper script (not a Python entry point) that currently only supports 1 node. The underlying Python entry point seems to be ami-remote. (2) The claim "ami-local does not support multiple workers with psana" — I couldn't find this restriction in the code (-n accepts any value with any source). Is this a practical limitation rather than a code-enforced one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. you would not run ami-remote directly, thats kind of the equivalent of running ami-worker or ami-node. ami-mpi is the equivalent of running ami-local. ive forgotten the reason we can't run more than 1 node with ami-mpi, it was something to do with starting the local collectors i think?

  2. this is a limitation of psana. ami-local -n uses multiprocessing, so ami offline with psana effectively makes every worker see every single event, you are not partitioning events among the workers since psana needs mpi for that. maybe we should have a check if you run ami-local -n (more than 1 worker) psana:// to just print a message to use ami-mpi.

Choose a reason for hiding this comment

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

I see. Could we add notes somewhere to highlight these two points?

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