Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔨 Move chart configs into chart_configs table #2957

Merged
merged 6 commits into from
Sep 3, 2024
Merged

Conversation

Marigold
Copy link
Collaborator

@Marigold Marigold commented Jul 10, 2024

TODO

  • Update ORM
  • Update all queries with strings from charts and join charts
  • Check all usages of type and slug in queries

TODO before merging

  • Check that ORM is up to date with MySQL schema

Testing

  • Test chart-diff
  • Test upserts to MySQL in ETL

@Marigold Marigold force-pushed the db-chart-configs branch 2 times, most recently from dd4741c to da01cfc Compare July 10, 2024 09:51
@Marigold
Copy link
Collaborator Author

@sophiamersmann this is ready for review (though not tested properly, yet). Let me know when you're ready or when you change schema.

@sophiamersmann
Copy link
Member

@Marigold the grapher pr also needs to go through review first (and I need to do another round of testing), so it won't be before next week :)

@owidbot
Copy link
Contributor

owidbot commented Jul 15, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-db-chart-configs

chart-diff: ✅ No charts for review.
data-diff: ❌ Found differences
= Dataset garden/artificial_intelligence/2024-08-05/epoch_aggregates_affiliation
  = Table epoch_aggregates_affiliation
    ~ Column cumulative_count (changed metadata)
-       -   Describes the sector where the authors of a notable AI system have their primary affiliations. The 2024 data is incomplete and was last updated 05 August 2024.
        ?                                                                                                                                                   ^^^^^^^^
+       +   Describes the sector where the authors of a notable AI system have their primary affiliations. The 2024 data is incomplete and was last updated 2 September 2024.
        ?                                                                                                                                                   ^^^^^ +++++
    ~ Column yearly_count (changed metadata)
-       -   Describes the sector where the authors of a notable AI system have their primary affiliations. The 2024 data is incomplete and was last updated 05 August 2024.
        ?                                                                                                                                                   ^^^^^^^^
+       +   Describes the sector where the authors of a notable AI system have their primary affiliations. The 2024 data is incomplete and was last updated 2 September 2024.
        ?                                                                                                                                                   ^^^^^ +++++


Legend: +New  ~Modified  -Removed  =Identical  Details
Hint: Run this locally with etl diff REMOTE data/ --include yourdataset --verbose --snippet

Automatically updated datasets matching weekly_wildfires|excess_mortality|covid|fluid|flunet|country_profile|garden/ihme_gbd/2019/gbd_risk are not included

Edited: 2024-09-03 06:50:58 UTC
Execution time: 12.39 seconds

@sophiamersmann
Copy link
Member

Hey @Marigold, the chart_configs PR on the Grapher side should be ready to merge soon-ish (this or next week). How do things look on the ETL side? (I'd be happy to look over this PR if you want me to)

@Marigold
Copy link
Collaborator Author

Marigold commented Aug 7, 2024

@sophiamersmann all look good, both schema renames and calling Admin endpoint. Ready whenever you are!

etl/grapher_model.py Outdated Show resolved Hide resolved
etl/grapher_model.py Outdated Show resolved Hide resolved
sophiamersmann added a commit to owid/owid-grapher that referenced this pull request Sep 3, 2024
## Summary

Adds a `chart_configs` table to the database, adds a `configId` column to the `charts` table that points to a `chart_configs` row, and removes the `config` column.

Shouldn't result in any functionality change.

## Details

- All configs are copied from the `charts` table to the `chart_configs` table (for now, `patch config = full config` since inheritance hasn't been implemented yet)
- Configs have a UUID, but it's not yet used (the id of the chart is still the sequential primary key of the charts table) 

## Testing

- [x] Bakes locally
- [x] Bakes staging site

Site:
- [x] http://staging-site-db-chart-configs/
- [x] http://staging-site-db-chart-configs/grapher/life-expectancy
- [x] http://staging-site-db-chart-configs/explorers/coronavirus-data-explorer
- [x] http://staging-site-db-chart-configs/explorers/democracy
- [x] http://staging-site-db-chart-configs/explorers/conflict-data
- [x] http://staging-site-db-chart-configs/poverty
- [x] http://staging-site-db-chart-configs/financing-education
- [x] http://staging-site-db-chart-configs/part-two-how-many-people-die-from-extreme-temperatures-and-how-could-this-change-in-the-future
- [x] http://staging-site-db-chart-configs/covid-hospitalizations
- [x] http://staging-site-db-chart-configs/country/germany
- [x] http://staging-site-db-chart-configs/energy/country/india
- [x] http://staging-site-db-chart-configs/data-insights
- [x] http://staging-site-db-chart-configs/collection/top-charts
- [x] http://staging-site-db-chart-configs/sdgs
- [x] http://staging-site-db-chart-configs/sdgs/no-poverty

Admin:
- [x] Chart editor:
    - [x] Create new chart
    - [x] Save as new chart
    - [x] Update existing chart
    - [x] Publish chart
    - [x] Unpublish chart
    - [x] Delete chart
- [x] Bulk chart editor
- [x] Chart test page
- [x] Variable page (chart list)

## On the ETL side

Mojmir opened a PR for changes to the ETL: owid/etl#2957

## To do

- [x] ~Is it safe to rewrite the config's id?~ I think it's okay
- [x] I don't understand why we manually insert `createdAt` and `updatedAt` since the db inserts values on insert/update?
- [x] Checklist for updating the db: https://www.notion.so/owid/Database-access-in-Grapher-d9db343c2bfb4ae0b14b3dec72f686c6?pvs=4#dd5b51bab6f84630839f4173b59feba2
- [x] Let Mojmir know so that he can update the ETL db schema (wait for the table structure to be signed off on)

## To do after merging

- [ ] ETL: owid/etl#2957
- [ ] Datasette: owid/analytics#138
- [ ] Schema validation: owid/automation#12
@Marigold Marigold merged commit 688c434 into master Sep 3, 2024
5 of 7 checks passed
@Marigold Marigold deleted the db-chart-configs branch September 3, 2024 07:27
@Marigold Marigold restored the db-chart-configs branch September 3, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants