Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…emoryStore-Problem # Conflicts: # uv.lock
Saaketh0
added
CRITICAL
…emoryStore-Problem
CI format:check failed after the main merge pulled in unformatted code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
userAugustos
changed the title
userAugustos
changed the title
userAugustos
changed the title
There was a problem hiding this comment.
Suggestion on direction: a database isn't an agent, and declaring it in agents: makes it inherit the whole agent path the 50051 port mapping, the CANYONOS_AGENT_* env, the LLM proxy env, the health gate, the metrics polling which this PR then switches off with elif type == "database" in four files. The _wait_for_healthy timeout on every deploy, the empty EC2 health check, the unused 50051 mapping and the dead endpoint in the Local record all come from that choice.
The part that does work is the routing table: the address is published under the service name and reachable from inside the network, as the docs promise. Keep that. What changes is where it's declared and how it comes up: a top-level section of its own (services:, say) carrying image, port, volume_path, env, provisioned on a path that checks readiness on the real port instead of 50051 — the way _redis_container_healthy (global_controller.py:409) already does for Redis — and that still publishes into routing_table:endpoints under the name. No replicas, no agent env, no stub, no metrics.
Worth supporting the external case in the same section it's the cheaper half: resolve_database_url (telemetry_logging.py:91) resolves a URL from config or env, and CANYONOS_DATABASE_URL is already injected into workflow containers (Local/_runtime.py:150). Someone with RDS or an existing Postgres should be able to point at it and skip provisioning. That also settles where credentials come from, which the docs leave open the adapter resolves the address from the routing table but never the user and password, so they'd end up duplicated in env_file
| elif spec.get("type") == "database": | ||
| instance["container_port"] = "5432" | ||
| instance["host_port"] = "5432" | ||
| instance["endpoint"] = f"{host}:5432" |
There was a problem hiding this comment.
_bootstrap_instance maps -p {db_port}:5432, but endpoint and host_port are fixed at 5432 here. routing_endpoint_for returns this endpoint, so with db_port: 5433 every agent resolving the service from routing_table:endpoints gets {host}:5432, where nothing is listening. spec.get("db_port", 5432) for both, keeping container_port at the image's own port.
| instance["public_host"] = provisioned["public_host"] | ||
| if spec.get("type") == "workflow": | ||
| instance["api_port"] = str(spec.get("api_port", 8080)) | ||
| elif spec.get("type") == "database": |
There was a problem hiding this comment.
The health check this branch passes through doesn't test the database. _check_controller_health at line 177 probes {host}:50051, the agent's gRPC port, and a database container has no process on it the probe only succeeds because -p 50051:50051 leaves a docker-proxy listener accepting on the host. With userland-proxy=false the same call times out and terminate_instance tears the instance down. Probing the mapped database port would make the gate mean something, and the 50051 mapping can go.
| runtime_id, | ||
| # Docker Desktop resolves this automatically; native Linux Docker | ||
| # (e.g. an EC2 test box) does not unless told to. | ||
| "--add-host", |
There was a problem hiding this comment.
This breaks two tests tests/test_instance_manager_runtime.py compares the whole docker run argv and the two new elements land before -p. test_local_instances_keep_default_host_and_increment_host_ports and test_local_workflow_and_resource_flags_stay_the_same pass on main and fail here. references/llm-proxy.md:38 also still says host.docker.internal doesn't resolve in these containers and to never advise it. Unrelated to the database type, so cleaner on its own, with the tests and the doc updated alongside.
| if project_id: | ||
| cmd.extend(["-e", f"CANYONOS_PROJECT_ID={project_id}"]) | ||
| elif ctrl_type == "database": | ||
| cmd.extend(["-p", f"{spec.get('db_port', 5432)}:5432"]) |
There was a problem hiding this comment.
The retry loop above only bumps host_port; this mapping stays {db_port}:5432 on every pass. With a Postgres already on 5432 on the machine or replicas: 2, which nothing rejects all 50 attempts fail identically and the error reads no free port found after 50 attempts, pointing at the wrong port. api_port is checked with _port_bound before the loop for this same reason, and db_port can use it
| volume_path = spec.get("volume_path") | ||
| if volume_path: | ||
| cmd.extend(["-v", f"canyonos-{agent_name.lower()}-data:{volume_path}"]) | ||
| for key, value in spec.get("env", {}).items(): |
There was a problem hiding this comment.
env is assumed to be a mapping here while the validator accepts any value for it, so env: "POSTGRES_PASSWORD=x" raises 'str' object has no attribute 'items' in the middle of the bootstrap. EC2/_runtime.py:327 is the same shape. Better to reject a non-mapping env in manifest.py so it fails at validation instead of at deploy
| def _openai_embedding_stub(body): | ||
| """A minimal embeddings response, one canned vector per requested input.""" | ||
| try: | ||
| count = len(json.loads(body or b"{}").get("input") or [None]) |
There was a problem hiding this comment.
input can be a string, not only a list, the OpenAI embeddings API accepts both. len() over a string counts characters, so {"input": "hello world"} comes back with 11 vectors instead of 1, and anything pairing inputs with embeddings reads the wrong mapping, including the LangGraph store this PR is for. Wrap a bare string in a list before counting
There was a problem hiding this comment.
@Saaketh0 U can ignore if this scenario is not actually possible, worth to know
| """A minimal embeddings response, one canned vector per requested input.""" | ||
| try: | ||
| count = len(json.loads(body or b"{}").get("input") or [None]) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
A body that parses to something other than a mapping still reaches .get, and AttributeError isn't in the tuple, so a body of [1, 2] gives a 500 instead of the canned response
| continue | ||
| target_image = f"canyonos-{agent_name.lower()}" | ||
| logger.info("Pulling database image '%s' as '%s'", image, target_image) | ||
| subprocess.run(["docker", "pull", image], check=True) |
There was a problem hiding this comment.
The builds in this command go through _docker_build, which passes --platform {_docker_platform()} (cli.py:87), but this pull takes the host's architecture. On an arm64 machine the tagged canyonos-{name} is arm64, and the EC2 transfer the comment right above mentions puts an image on an x86_64 host that can't exec it. Passing --platform, _docker_platform() keeps it consistent with the rest of the build
| valid = False | ||
| if service_type == "database": | ||
| image = entry.get("image") | ||
| if not isinstance(image, str) or not image.strip(): |
There was a problem hiding this comment.
Only image is checked; db_port, volume_path, env and replicas go through untouched. db_port: "not-an-int" becomes -p not-an-int:5432 and dies at docker run, env: "NOPE" dies at bootstrap, and replicas: 3 starts three separate containers behind one routing entry, splitting the writes the feature exists to keep in one place. Cheaper to catch here than at deploy
3 participants
Added a new type of agent apart from "agent" and "workflow", called "database". It is skipped from the entire stub process and has separate variables inside the global_controller.yaml file to allow for connections and initializations.
Instead of creating the image, it would also pull the image from the internet.
This would allow us to connect/interface with external databases and memory stores.
Wrote some basic edits to skill file, @nickhuo review and keep/discard them.