🎉 Charts as first-class ETL citizens via simple mdims#5969
🎉 Charts as first-class ETL citizens via simple mdims#5969pabloarosado wants to merge 10 commits into
Conversation
|
Quick links (staging server):
Login: chart-diff: ❌
Edited: 2026-04-28 05:46:09 UTC |
pabloarosado
changed the title
|
@codex review |
Marigold
left a comment
Marigold
left a comment
There was a problem hiding this comment.
Neat, I'm surprised how elegant this is compared to the previous PR. After you merge it, I'll make it work with Chart Preview in VSCode.
lucasrodes
left a comment
lucasrodes
left a comment
There was a problem hiding this comment.
Awesome, Pablo! Thanks for working and pushing for this. I've left two comments. One more on the "code design" side of things; the other is a minor concern about the source of truth/conflicts with chart configs.
| if existing is not None: | ||
| # Preserve publication state unless the user explicitly overrode it. | ||
| config.setdefault("isPublished", existing.config.get("isPublished", False)) | ||
| log.info("collection.chart.update", slug=slug, chart_id=existing.id) | ||
| admin_api.update_chart(chart_id=existing.id, chart_config=config, user_id=user_id) | ||
| chart_id = existing.id |
There was a problem hiding this comment.
So, if a chart already exists in DB, we can just overwrite it via ETL? Am I understanding it correctly?
Couldn't this cause unwanted overwrites? Say the chart exists in DB and is live (i.e., this if evaluates to True). If the config in ETL differs from that in the DB (because someone has edited it in either ETL or the DB), wouldn't the DB version always be overwritten?
I see that on staging, one can reject changes in the Chart diff (I suppose), but the ETL config still appears and may have been merged into master. Then, in master, if etl runs, the DB chart would be overwritten, no?
I'm just not sure I understand this logic. At the moment, it feels a bit dangerous, but I could be misunderstanding it.
Thanks Mojmir! In principle Chart Preview should already work in this PR. I tweaked a few things and checked on a couple of examples, but I haven't checked thoroughly. |
|
Closing this PR without merging; the good thing of the current approach is that no changes were required in owid-grapher. However, after various discussions, it seems that having a proper layered approach (making some changes in owid-grapher) is preferrable. |
4 participants
Summary
Allow charts to be created and edited from ETL, as a special case of mdims (instead of creating a new
Graphobject, as in this old PR).Additionally:
etl/steps/export/multidim/animal_welfare/latest/banning_of_chick_culling.config.yml.To be discussed