🐛 Layer-aware chart diff and sync for ETL-authored charts - #6581
🐛 Layer-aware chart diff and sync for ETL-authored charts#6581pabloarosado wants to merge 4 commits into
Conversation
|
Quick links (staging server):
Login: chart-diff: ✅No charts for review.data-diff: ✅ no differencesNo differences found. Automatically updated datasets matching excess_mortality|covid|fluid|flunet|country_profile|garden/ihme_gbd/2019/gbd_risk are not included. Run locally with Edited: 2026-08-26 17:25:56 UTC |
Four scenarios showing what this PR changes in practice. The cast: DS1 is a data scientist who created a chart from ETL (the chart's configuration lives in a YAML file in this repository, and the chart is live on the website). DS2 is another data scientist with an open pull request. Admin is a colleague who edits charts in the production chart admin. Two background facts that all scenarios rely on:
Scenario 1: a typo is fixed in production while an unrelated pull request is openAdmin notices a typo in the live chart's subtitle and fixes the typo directly in the production chart admin. DS2 has an open pull request about something unrelated, say a tourism dataset. DS2's staging server rebuilt DS1's chart from the YAML file, so the staging copy still has the typo.
Scenario 2: a data update rebuilds the chart while production carries a fixDS2's pull request updates the dataset that DS1's chart plots. The staging server rebuilds the chart so that it points at the new version of the indicators. Production, meanwhile, carries Admin's subtitle fix.
Scenario 3: someone edits an ETL chart in the staging adminDuring review, DS1 changes the chart's y-axis in the staging server's chart admin, instead of changing the YAML file.
Scenario 4: a chart is approved, then edited in production before the mergeOn Monday, DS2 approves the chart in chart-diff. The pull request is not merged yet. On Wednesday, Admin edits the same chart in the production admin. On Friday, the pull request is merged.
🤖 Generated with Claude Code |
|
@pabloarosado I've refreshed the 3 PRs (2 in grapher, one here) with recent additions. Can you refresh this PR to adapt to the changes made on the PR below and the grapher api changes that might be relevant? |
An ETL-authored chart is two layers with two owners: the etlConfig (owned by git, delivered by the production ETL) and the admin patch (owned by admins, delivered by chart-sync). Comparing and syncing whole configs made production admin hotfixes look like branch changes on unrelated staging servers, and let an approved sync silently overwrite them. - chart-diff compares ETL-managed charts layer by layer: a chart is listed only when the branch changed a layer (code layer differs, or the staging patch was deliberately edited). - chart-sync writes the admin patch only when the staging patch is non-pristine; a pristine patch never overwrites production's. - Cross-env twin conflicts are re-enabled whenever the staging patch carries edits, and approvals now bind to the production timestamp they reviewed, so later production edits invalidate them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
For ETL-authored charts, the branch's medium is the config YAML; edits made in the staging admin are legal but should be a conscious choice. Chart-diff now shows a warning when an ETL chart carries deliberate staging admin edits that differ from production, explaining they will sync as an admin override and suggesting to move them into the YAML. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
owid/owid-grapher#6826 renamed charts.configIdETL to charts.patchConfigIdETL (an authored layer, like charts.patchConfigId and variables.patchConfigIdETL) and charts.catalogPath to charts.etlConfigCatalogPath (the step that owns the ETL layer, not an identifier). Rename the ORM columns and every read of them in chart-diff, chart-sync and the chart-config-id CLI, and check for the new names when probing whether production has the columns yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0957536 to
0b60800
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous staging build ran the ETL export step against the old admin process while the grapher build was still restarting it, so the by-config etlConfig route returned 404. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Thanks Daniel, done. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6772a2ee72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if patch.get("isPublished"): | ||
| return False |
There was a problem hiding this comment.
When an ETL-authored chart was already published before the staging server was created, its source patch legitimately contains isPublished: true; this makes every such patch non-pristine. If production then receives an admin-only hotfix, etl_chart_has_branch_changes interprets the resulting patch difference as a branch change, and chart-sync sets push_admin_patch and can replace the production patch with staging's version—the exact phantom-diff/overwrite scenario this change is intended to prevent. Track whether the patch was edited on this staging branch rather than treating a longstanding publication flag as evidence of a staging edit.
Useful? React with 👍 / 👎.
| target_df = read_sql( | ||
| base_q(target_session) + where, target_session, params={"chart_ids": tuple(source_df.chartId.unique())} | ||
| ) |
There was a problem hiding this comment.
For cross-environment twins—the normal case described here where staging and production have different numeric IDs but share a config UUID—this query searches production using the staging numeric ID. The target row is therefore absent or unrelated, target_df["chartChecksum"] is null, and the new layer-aware comparison is never executed, so production-only hotfixes still appear as phantom changes. Resolve target rows by configId (or etlConfigCatalogPath) and align them back to the source chart IDs before comparing layers.
Useful? React with 👍 / 👎.
| if not etl_chart_has_branch_changes( | ||
| json.loads(source_etl_raw), json.loads(target_etl_raw), source_patch, target_patch | ||
| ): |
There was a problem hiding this comment.
When an ETL-managed chart changes only isInheritanceEnabled, the checksum differs, but this branch compares only the ETL and patch JSON layers and marks the configs equal because the table-level flag is passed to neither side. The chart consequently disappears from chart-diff; moreover, a pristine patch makes chart-sync skip update_chart, so the changed flag would not reach production even if another change selected the chart. Include isInheritanceEnabled in the layer-aware decision and ensure such a change triggers the chart update.
Useful? React with 👍 / 👎.
|
Closing this PR. The four scenarios this PR was meant to fix work already on the base branch, after the column renames (done in #6765). The additional layer-aware logic is not needed. |
3 participants
Stacked on #6511 "Charts as first-class citizens in ETL, addressed by chart config UUID". Fixes how chart-diff and chart-sync treat ETL-authored charts, so that production admin edits can never be silently overwritten and unrelated branches stop seeing phantom chart changes. ETL-only changes; no owid-grapher changes required.
The problem (two scenarios)
Phantom diffs on unrelated branches. An ETL-authored chart is rebuilt from its YAML by every staging server whose branch contains it. That rebuild carries no production admin edits, so the moment an admin hotfixes the chart in production, every staging server's copy differs from production and the chart shows up in every branch's chart-diff — as if the branch had changed it. The "diff" it shows is literally the hotfix being undone.
Silent overwrite of production hotfixes. For cross-environment twins (same config UUID, different numeric ids — the normal state of an ETL-authored chart), the edited-in-production conflict check was disabled unconditionally, and approvals were recorded with a NULL target timestamp that later production edits couldn't invalidate. Approving such a phantom diff and merging made chart-sync push the staging config over production, deleting the hotfix with no warning (flagged by Codex on #6511 "Charts as first-class citizens in ETL, addressed by chart config UUID" as P1).
The model
An ETL-authored chart is two layers with two owners and two delivery routes:
etlConfig(code layer)Chart-diff should list a chart only when the branch changed a layer; chart-sync should write only layers the branch changed.
What changed
_modified_chart_configs_on_staging): ETL-managed charts are compared layer by layer instead of full-vs-full. A chart is listed only if the code layers differ, or the staging patch is non-pristine and differs from production's. Production-only patch edits no longer surface on unrelated branches.slug,$schema,version,id) and unpublished counts as untouched. Publishing, or any config override, marks it as a deliberate admin edit.update_chart(which the server re-diffs into the target's patch) runs only when the staging patch is non-pristine — in both the update and the create paths. A pristine patch can never clobber production's._target_updated_at_for_reviewbinds approvals to the production timestamp that was reviewed, twins included, so a later production edit invalidates a stale approval.Chart.load_patch_config()added to the grapher model (mirrorsload_etl_config). After thechart_configsrefactor a chart's rendered config, admin patch and ETL layer are threechart_configsrows (configId,patchConfigId,patchConfigIdETL); the layer comparison reads each from its own row.charts.configIdETL→patchConfigIdETL,charts.catalogPath→etlConfigCatalogPath) in the ORM, chart-diff, chart-sync andetl chart-config-id. These touch #6511 "Charts as first-class citizens in ETL, addressed by chart config UUID"'s code and can be moved down to that branch.The staging-admin warning
For ETL-authored charts, the branch's medium is the YAML; edits made in the staging admin are legal but should be a conscious choice. When chart-diff finds an ETL chart with deliberate staging-admin edits that differ from production, it now shows a warning: the edits will sync as an admin override, and the reviewer is nudged to move them into the chart's
.config.yml(versioned, reviewed with the code). Long-standing overrides on adopted charts that are identical on both sides don't trigger it.Known limitations
charts.patchConfigIdETL(production before owid/owid-grapher#6826 "ETL-authored chart configs, addressed by chart config UUID" deploys), the comparison falls back to today's full-config behavior.Test plan
tests/apps/wizard/app_pages/chart_diff/test_chart_diff.py: pristine-patch detection, phantom hidden on unrelated branches, code-layer change listed, staging-admin edit listed, adopted chart with identical patches hidden, twin conflict gated on staging patch.make checkclean.🤖 Generated with Claude Code