fix: wire the lit orchestrator demo to its required subagents#838
fix: wire the lit orchestrator demo to its required subagents#838stablegenius49 wants to merge 1 commit intogoogle:mainfrom
Conversation
Fixes google#586 by giving the lit orchestrator sample default subagent URLs and starting the required restaurant/contact/rizzcharts agents on the documented ports when running demo:orchestrator. Validation: package.json script assertions via Node.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request successfully wires up the Lit orchestrator demo by adding and updating the necessary npm scripts in package.json. The changes appear to be functionally correct. My review includes two suggestions to improve the maintainability and robustness of these new scripts by reducing a very long command and eliminating duplicated port numbers.
| "serve:agent:restaurant:orchestrator": "cd ../../agent/adk/restaurant_finder && uv run . --port=10003", | ||
| "serve:agent:contact_lookup:orchestrator": "cd ../../agent/adk/contact_lookup && uv run . --port=10004", | ||
| "serve:agent:rizzcharts:orchestrator": "cd ../../agent/adk/rizzcharts && uv run . --port=10005", | ||
| "serve:agent:orchestrator": "cd ../../agent/adk/orchestrator && uv run . --port=10002 --subagent_urls=http://localhost:10003 --subagent_urls=http://localhost:10004 --subagent_urls=http://localhost:10005", |
There was a problem hiding this comment.
The ports for the subagents (10003, 10004, 10005) are hardcoded in this script and also in the serve:agent:*:orchestrator scripts on lines 13-15. This duplication means that if a port number changes, it must be updated in two places, which is error-prone and violates the DRY principle. Consider using a mechanism to define these ports only once. For example, you could use a tool like cross-env to set environment variables for the ports and reference them in the scripts, making them more robust and easier to maintain.
| "demo:restaurant": "npm install && npm run build:renderer && concurrently -k -n \"SHELL,REST\" -c \"magenta,blue\" \"npm run serve:shell\" \"npm run serve:agent:restaurant\"", | ||
| "demo:contact": "npm install && npm run build:renderer && concurrently -k -n \"SHELL,CONT1\" -c \"magenta,green\" \"npm run serve:shell\" \"npm run serve:agent:contact_lookup\"", | ||
| "demo:orchestrator": "npm install && npm run build:renderer && concurrently -k -n \"SHELL,ORCH\" -c \"magenta,cyan\" \"npm run serve:shell\" \"npm run serve:agent:orchestrator\"" | ||
| "demo:orchestrator": "npm install && npm run build:renderer && concurrently -k -n \"SHELL,REST,CONT,RIZZ,ORCH\" -c \"magenta,blue,green,yellow,cyan\" \"npm run serve:shell\" \"npm run serve:agent:restaurant:orchestrator\" \"npm run serve:agent:contact_lookup:orchestrator\" \"npm run serve:agent:rizzcharts:orchestrator\" \"npm run serve:agent:orchestrator\"" |
There was a problem hiding this comment.
This script definition is very long, which harms readability and maintainability. To make it cleaner, you could extract the logic for running the subagents into a separate helper script.
For example, you could add a serve:subagents:orchestrator script:
"serve:subagents:orchestrator": "concurrently -k -n \"REST,CONT,RIZZ\" -c \"blue,green,yellow\" \"npm run serve:agent:restaurant:orchestrator\" \"npm run serve:agent:contact_lookup:orchestrator\" \"npm run serve:agent:rizzcharts:orchestrator\""Then, the demo:orchestrator script would be much simpler:
"demo:orchestrator": "npm install && npm run build:renderer && concurrently -k -n \"SHELL,AGENTS,ORCH\" -c \"magenta,white,cyan\" \"npm run serve:shell\" \"npm run serve:subagents:orchestrator\" \"npm run serve:agent:orchestrator\""
Description
This wires the Lit orchestrator sample to the ports already documented in the orchestrator README so
npm run demo:orchestratorcan actually boot the full sample stack.What changed:
serve:agent:orchestratorto pass the default--subagent_urlsvalues the orchestrator requiresdemo:orchestratorto launch the shell plus all three subagents before starting the orchestratorFixes #586.
Pre-launch Checklist
Tests:
package.jsonscripts wire the documented ports and subagent URLs.