narratives: drop the cdc backend, and the deploy.sh shrink that follo… - #476
ddebasmita-lab wants to merge 4 commits into
Conversation
…wed it Ported from the source repo, where they are e02ac6f, ffd014c, ec1d4ce and 710b874. Taken as one commit because the second and third only make sense after the first. DATA_BACKEND is now dcp or none. Cloud SQL was the reason: no connection-pool sizing anywhere in the Terraform, a shared-core db-g1-small default tier, and Cloud Run concurrency high enough that MySQL failed with "too many connections" rather than merely slow queries. Spanner has no fixed connection ceiling, no NL server to run, managed ingestion, and a Mixer that defaults to stale reads so ingestion causes no downtime. Removed: the Cloud SQL instance, database and user; the data bucket; the ingest Job; the data-plane service and the app-to-data invoker binding; image/ and sample-data/; promote-image, which existed only to mirror datacommons-services:stable; and 18 Terraform variables nothing could reach. Also the "<instance>-runtime" service account and its seven IAM bindings -- that was the data-plane container's identity, cloudsql.client and objectAdmin on the data bucket, and no app-plane container has ever needed it. MAPS_API_KEY had the same single consumer, so supplying one now warns rather than storing a secret nothing reads. Kept deliberately. Direct VPC egress, whose derived default was is_cdc: a DCP plane can be a private service with internal ingress, which is what VPC egress is for, and that plane is provisioned elsewhere so its ingress is not knowable from here. MCP 1.2.x handling in the agent, which is payload-shape handling rather than cdc plumbing -- nothing guarantees every DCP instance serves 1.3.x. The "-datacommons" and "-data-ingest" suffixes in check-state-owner.py, because a state file written before this still contains those resources, and destroying one of those states is the case that guard exists to catch. deploy.sh rejects DATA_BACKEND=cdc with an explanation rather than a bare validation error: this revision cannot describe those resources, so it cannot destroy them either, and an instance still on that backend has to be torn down with the last revision that declared them. Then a sweep for what nothing uses. The app-plane image shipped two Gemini SDKs it never imported -- the agent calls Gemini over REST with requests -- and between them google-genai and google-generativeai pulled in httpx, pydantic, websockets, tqdm and the google-api-python-client stack. requirements drops from 48 packages to 29, and is generated now rather than hand-maintained, which surfaced two real dependencies of google-cloud-secret-manager that the old freeze was missing. Six branding keys were declared in the schema, shipped in every example and read by no code; two of them are worse than clutter, because analytics.ga_tag_id and extra_css could be set, synced, and silently do nothing. Then deploy.sh, 1146 lines to 609, no behaviour change. --preflight, --destroy, --config-only and --bootstrap-secrets are 341 lines that never run during a deploy, so they move to deploy/modes/ and are sourced -- they still share the loaded instance.env and the log helpers. Comments go from 266 lines to 115, each keeping its rule and its consequence and dropping the account of how it was found. Four lines of dead code go too, and monitoring.googleapis.com is now enabled: the module creates an alert policy and an uptime check, which works today only because the API is usually on already and would fail on a fresh project. The review fixes already on this branch are untouched -- the utf-8 config read and write, the urlparse prompt-URL derivation and its tests, and the dcproxy TODO. README keeps the test_prompt_urls line that came with them. Verified here after the port: terraform fmt and validate clean; bash -n clean on deploy.sh, smoke.sh and the four mode files; 45 agent checks across five suites; 108 UI tests in 9 files; tsc --noEmit and vite build clean.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the self-hosted Cloud SQL data plane (the cdc backend) and all of its associated resources, such as the services container, ingest job, and sample data, leaving only the dcp and none backends. It also refactors deploy.sh by modularizing its sub-commands into separate scripts under deploy/modes/ and cleans up unused dependencies in both the Python agent and the React UI. The review feedback identifies a critical bug where the deployment script fails to exit after running preflight checks, an issue with API key validation failing on transient Gemini API errors, a missing curl check in the CLI tool verification, and the omission of vite from the UI's devDependencies after its removal from dependencies.
| if [ "$PREFLIGHT" = true ]; then | ||
| run_preflight |
There was a problem hiding this comment.
If --preflight is specified, the script should exit after running run_preflight instead of continuing to the rest of the deployment. Currently, there is no exit statement, so running ./deploy.sh --preflight will perform the checks and then immediately proceed to execute the full deployment (configuring gcloud, enabling APIs, building images, running Terraform, etc.), which violates the 'create nothing' guarantee of preflight.
| if [ "$PREFLIGHT" = true ]; then | |
| run_preflight | |
| if [ "$PREFLIGHT" = true ]; then | |
| run_preflight || exit 1 | |
| exit 0 | |
| fi |
There was a problem hiding this comment.
Already handled — deploy.sh does exit. In the current branch it is lines 250-253:
if [ "$PREFLIGHT" = true ]; then
run_preflight
exit $?
fiexit $? propagates run_preflight's status, so a failed check exits non-zero and a clean one exits 0. There were four stray blank lines directly above this block, which is most likely what split the diff hunk and hid the exit from the review; they are gone now.
| "devDependencies": { | ||
| "@tailwindcss/vite": "^4.1.14", |
There was a problem hiding this comment.
The primary build tool vite was removed from dependencies but was not added to devDependencies. While it might be transitively installed by @tailwindcss/vite or @vitejs/plugin-react, relying on transitive dependencies for your main build tool is risky and can lead to build failures or version mismatches. It should be explicitly declared in devDependencies.
| "devDependencies": { | |
| "@tailwindcss/vite": "^4.1.14", | |
| "devDependencies": { | |
| "vite": "^6.2.0", | |
| "@tailwindcss/vite": "^4.1.14", |
There was a problem hiding this comment.
vite is already in devDependencies, at line 34 of ui/package.json. This PR moved it there from dependencies, along with @tailwindcss/vite and @vitejs/plugin-react. Applying the suggestion would add a second vite key to the same object.
Two fixes from the review bot on this PR. bootstrap-secrets.sh validates both API keys with curl, but the early tool check did not ask for it, so a machine without curl got past the check and failed mid-run. The Gemini key check treated every non-200 as a rejected key, so a 429 or a 5xx aborted the deploy and told you the key was bad. Only 400, 401 and 403 say that; everything else now warns and stores the key. This is what the DC_API_KEY check a few lines above already does. Also drops four blank lines the mode extraction left behind.
… that Documents the agent-config document, which had a row in the override table and nothing else -- no section, no example to copy, and no mention that schemas/agent-config.example.json exists. The thinking levels, the model choices, the synthesis-only fallback and the query_param_key gate were undocumented, and the four steps to turn on the knowledge base lived only in a _comment inside defaults/agent-config.json. Drops provisioning a new DCP data plane. This repository attaches to an instance that is already running; standing one up is done elsewhere, with datacommons-cli, and documenting it here implied otherwise. Attaching is now the single place that explains dcp. Drops local development, testing, verifying a deployment, design notes and contributing, leaving the twelve sections a deployer needs. This removes the test_prompt_urls.py line from the Testing section along with the section itself. Fixes what the cdc removal and the deploy-mode extraction left stale: the intro still offered a Cloud SQL plane, the repository layout listed promote-image.yaml and a docs/architecture.drawio that no longer exists and omitted deploy/modes/, and curl was missing from the prerequisites now that it validates the API keys.
juliawu
left a comment
juliawu
left a comment
There was a problem hiding this comment.
Thank you, this is a major cleanup!
Could you separate out the deploy.sh refactoring into a separate, followup PR? This PR is large enough already, and it'll be easier for me to leave dedicated comments if they're in their own PR.
Also, could you reformat the PR description? It looks like an AI agent dumped some thinking but didn't actually fill out the PR template. I especially want to see some instructions for how to test out these changes on my end.
Separately, I find the comments left by the Agent a little too quippy but not as understandable. Let's do some rewriting/refactoring of documentation comments as a followup for later.
| "analytics": { | ||
| "ga_tag_id": "" | ||
| }, | ||
| "splash_assets": [], |
There was a problem hiding this comment.
For my own understanding, why are analytics and splash_assets also removed here??
There was a problem hiding this comment.
Because nothing reads them. Neither key is in schemas/branding.schema.json, neither is in defaults/branding.json. They were dead keys carrying empty values (ga_tag_id: "" and splash_assets: []).
| "name": "Example Data Commons", | ||
| "region": "the world", | ||
| "states_term": "regions", | ||
| "fiscal_year_start": "01-01" |
There was a problem hiding this comment.
Also just a question for my own understanding, why is fiscal_year_start being removed?
There was a problem hiding this comment.
Same reason — nothing substitutes it. template_vars is an open map (additionalProperties: {type: string}), so any key you add becomes {{instance.<key>}}. The schema names three by way of documentation: name, region and states_term. Those three are the ones actually referenced by a shipped prompt, in defaults/prompts/synthesis.md lines 4-5.
No prompt has ever contained {{instance.fiscal_year_start}}, so as a default it rendered nowhere. It survives as the example of a user-defined key in agent/tests/test_prompt_rendering.py, which is the role it was really playing. A deployment that wants it adds it to its own config/agent-config.json and references it from its own prompt.
| # Resource limits for the app plane. The 4 vCPU / 16Gi pair that used to sit here | ||
| # belonged to the cdc services container, whose NL embeddings load needed it; no | ||
| # container in this deployment does. |
There was a problem hiding this comment.
We don't need to mention the old resource limits. Just use "# Resource limits for the app plane" as the comment here.
There was a problem hiding this comment.
Should this still say CUSTOM DATA COMMONS at the top?
There was a problem hiding this comment.
The product is still Custom Data Commons, so the name stays, but you are right that the line was overblown. It now reads:
# ===========================================================================
# Custom Data Commons — deploy and update one instance.
# ==========================================================================="UNIFIED ONE-COMMAND DEPLOYER & UPDATE MANAGER" in caps was the part not earning its place.
…rong
Addresses the review.
Asked for directly:
- requirements.txt: drop the header comment
- deploy-stamp.yaml: restore the missing step 1 and renumber 3-6 to 2-5
- pr-validate.yaml: number build-agent as step 4
- bootstrap-secrets.sh: stop indenting a top-level comment
- service_account.tf: drop the history of the removed runtime identity
- new-instance.tfvars.sample: "Resource limits for the app plane"
- README: drop "(Spanner)" and "No code changes, no branch."
- deploy.sh: the banner no longer shouts
Comments that were left wrong by the cdc removal:
- main.tf's header described two Cloud Run services, Cloud SQL, a data
bucket and MAPS_API_KEY/DB_PASS. There is one service and two secrets.
- An empty "Cloud SQL" section header, and a "data bucket (created here)"
header over a lookup of the config bucket, which is not created here.
- backend.tf documented the state prefix as cdc/<instance>; it is
custom-datacommons/<instance>.
- Three pointers to DEPLOY.md and docs/deployment.md, neither of which
exists. deploy.sh does that work.
- var.secret_prefix, which no longer exists.
Everywhere else the comments kept the rule and dropped the war story: what
broke, when, and how long it took to find is not what the next reader needs.
Net 225 lines of comment removed, no code touched outside the two README
lines above.
Verified: bash -n on every script, terraform fmt, tsc, 4/4 agent suites,
108/108 vitest, and the UI build.
2 participants
Overview
DATA_BACKENDbecomesdcpornone. This drops the self-hosted Cloud SQLdata plane (
cdc) and everything that existed only to serve it.Cloud SQL was the reason. There is no connection-pool sizing anywhere in the
Terraform, the default tier is a shared-core
db-g1-small, and Cloud Runconcurrency is high enough that MySQL returned "too many connections" rather
than merely getting slow. Spanner has no fixed connection ceiling, no NL server
to run alongside it, managed ingestion, and a Mixer that defaults to stale
reads, so ingestion causes no downtime.
The app plane does not change. It receives a data-plane URL and an auth mode,
and it has never cared which kind of plane is on the other end.
Changes Made
Removed with the
cdcbackendimage/andsample-data/promote-image.yaml, which existed only to mirrordatacommons-services:stable<instance>-runtimeservice account and its seven IAM bindings. That wasthe data-plane container's identity —
cloudsql.clientandobjectAdminonthe data bucket — and no app-plane container has ever needed it.
MAPS_API_KEYhad the same single consumer, so supplying one now warnsinstead of storing a secret nothing reads.
deploy.shRefactored deploy.sh with no behavior changes by removing dead code and extracting lines of non-deploy flags (--preflight, --destroy, --config-only, --bootstrap-secrets) into sourced scripts under deploy/modes/.