🔨 Remove --step-update and make committing explicit #3205
Conversation
|
Quick links (staging server):
Login: chart-diff: ✅No charts for review.data-diff: ❌ Found differences= Dataset garden/who/latest/monkeypox
= Table monkeypox
~ Dim country
- - Removed values: 43 / 85208 (0.05%)
date country
2024-07-11 Cote d'Ivoire
2024-07-17 Cote d'Ivoire
2024-08-03 Cote d'Ivoire
2024-08-06 Cote d'Ivoire
2024-08-12 Cote d'Ivoire
~ Dim date
- - Removed values: 43 / 85208 (0.05%)
country date
Cote d'Ivoire 2024-07-11
Cote d'Ivoire 2024-07-17
Cote d'Ivoire 2024-08-03
Cote d'Ivoire 2024-08-06
Cote d'Ivoire 2024-08-12
~ Column annotation (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date annotation
Cote d'Ivoire 2024-07-11
Cote d'Ivoire 2024-07-17
Cote d'Ivoire 2024-08-03
Cote d'Ivoire 2024-08-06
Cote d'Ivoire 2024-08-12
~ Column iso_code (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date iso_code
Cote d'Ivoire 2024-07-11 CIV
Cote d'Ivoire 2024-07-17 CIV
Cote d'Ivoire 2024-08-03 CIV
Cote d'Ivoire 2024-08-06 CIV
Cote d'Ivoire 2024-08-12 CIV
~ Column new_cases (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_cases
Cote d'Ivoire 2024-07-11 0
Cote d'Ivoire 2024-07-17 0
Cote d'Ivoire 2024-08-03 0
Cote d'Ivoire 2024-08-06 0
Cote d'Ivoire 2024-08-12 0
~ Changed values: 5 / 85208 (0.01%)
country date new_cases - new_cases +
Africa 2024-07-07 157 156
Africa 2024-07-28 220 218
Africa 2024-08-04 97 94
Africa 2024-08-11 88 84
Africa 2024-08-18 90 72
~ Column new_cases_per_million (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_cases_per_million
Cote d'Ivoire 2024-07-11 0.0
Cote d'Ivoire 2024-07-17 0.0
Cote d'Ivoire 2024-08-03 0.0
Cote d'Ivoire 2024-08-06 0.0
Cote d'Ivoire 2024-08-12 0.0
~ Changed values: 5 / 85208 (0.01%)
country date new_cases_per_million - new_cases_per_million +
Africa 2024-07-07 0.105 0.104
Africa 2024-07-28 0.147 0.146
Africa 2024-08-04 0.065 0.063
Africa 2024-08-11 0.059 0.056
Africa 2024-08-18 0.060 0.048
~ Column new_cases_smoothed (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_cases_smoothed
Cote d'Ivoire 2024-07-11 0.14
Cote d'Ivoire 2024-07-17 0.00
Cote d'Ivoire 2024-08-03 0.29
Cote d'Ivoire 2024-08-06 0.43
Cote d'Ivoire 2024-08-12 0.57
~ Changed values: 35 / 85208 (0.04%)
country date new_cases_smoothed - new_cases_smoothed +
Africa 2024-08-06 13.86 13.43
Africa 2024-08-10 13.86 13.43
Africa 2024-08-12 12.57 12.00
Africa 2024-08-18 12.86 10.29
Africa 2024-08-19 12.86 10.29
~ Column new_cases_smoothed_per_million (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_cases_smoothed_per_million
Cote d'Ivoire 2024-07-11 0.005
Cote d'Ivoire 2024-07-17 0.000
Cote d'Ivoire 2024-08-03 0.010
Cote d'Ivoire 2024-08-06 0.014
Cote d'Ivoire 2024-08-12 0.019
~ Changed values: 7 / 85208 (0.01%)
country date new_cases_smoothed_per_million - new_cases_smoothed_per_million +
Africa 2024-08-18 0.009 0.007
Africa 2024-08-19 0.009 0.007
Africa 2024-08-20 0.009 0.007
Africa 2024-08-21 0.009 0.007
Africa 2024-08-24 0.009 0.007
~ Column new_deaths (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_deaths
Cote d'Ivoire 2024-07-11 0
Cote d'Ivoire 2024-07-17 0
Cote d'Ivoire 2024-08-03 0
Cote d'Ivoire 2024-08-06 0
Cote d'Ivoire 2024-08-12 0
~ Changed values: 1 / 85208 (0.00%)
country date new_deaths - new_deaths +
Africa 2024-08-18 1 0
~ Column new_deaths_per_million (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_deaths_per_million
Cote d'Ivoire 2024-07-11 0.0
Cote d'Ivoire 2024-07-17 0.0
Cote d'Ivoire 2024-08-03 0.0
Cote d'Ivoire 2024-08-06 0.0
Cote d'Ivoire 2024-08-12 0.0
~ Changed values: 1 / 85208 (0.00%)
country date new_deaths_per_million - new_deaths_per_million +
Africa 2024-08-18 0.00067 0.0
~ Column new_deaths_smoothed (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_deaths_smoothed
Cote d'Ivoire 2024-07-11 0.0
Cote d'Ivoire 2024-07-17 0.0
Cote d'Ivoire 2024-08-03 0.0
Cote d'Ivoire 2024-08-06 0.0
Cote d'Ivoire 2024-08-12 0.0
~ Changed values: 7 / 85208 (0.01%)
country date new_deaths_smoothed - new_deaths_smoothed +
Africa 2024-08-18 0.14 0.0
Africa 2024-08-19 0.14 0.0
Africa 2024-08-20 0.14 0.0
Africa 2024-08-21 0.14 0.0
Africa 2024-08-24 0.14 0.0
~ Column new_deaths_smoothed_per_million (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date new_deaths_smoothed_per_million
Cote d'Ivoire 2024-07-11 0.0
Cote d'Ivoire 2024-07-17 0.0
Cote d'Ivoire 2024-08-03 0.0
Cote d'Ivoire 2024-08-06 0.0
Cote d'Ivoire 2024-08-12 0.0
~ Changed values: 7 / 85208 (0.01%)
country date new_deaths_smoothed_per_million - new_deaths_smoothed_per_million +
Africa 2024-08-18 0.00009 0.0
Africa 2024-08-19 0.00009 0.0
Africa 2024-08-20 0.00009 0.0
Africa 2024-08-21 0.00009 0.0
Africa 2024-08-24 0.00009 0.0
~ Column total_cases (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date total_cases
Cote d'Ivoire 2024-07-11 1
Cote d'Ivoire 2024-07-17 1
Cote d'Ivoire 2024-08-03 3
Cote d'Ivoire 2024-08-06 6
Cote d'Ivoire 2024-08-12 10
~ Changed values: 50 / 85208 (0.06%)
country date total_cases - total_cases +
Africa 2024-07-09 5065 5064
Africa 2024-07-17 5239 5238
Africa 2024-07-18 5239 5238
Africa 2024-08-04 5787 5781
Africa 2024-08-17 5875 5865
~ Column total_cases_per_million (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date total_cases_per_million
Cote d'Ivoire 2024-07-11 0.034
Cote d'Ivoire 2024-07-17 0.034
Cote d'Ivoire 2024-08-03 0.101
Cote d'Ivoire 2024-08-06 0.202
Cote d'Ivoire 2024-08-12 0.337
~ Changed values: 41 / 85208 (0.05%)
country date total_cases_per_million - total_cases_per_million +
Africa 2024-07-16 3.501 3.500
Africa 2024-07-26 3.653 3.652
Africa 2024-08-10 3.861 3.857
Africa 2024-08-14 3.919 3.912
Africa 2024-08-20 3.977 3.959
~ Column total_deaths (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date total_deaths
Cote d'Ivoire 2024-07-11 0
Cote d'Ivoire 2024-07-17 0
Cote d'Ivoire 2024-08-03 0
Cote d'Ivoire 2024-08-06 0
Cote d'Ivoire 2024-08-12 0
~ Changed values: 8 / 85208 (0.01%)
country date total_deaths - total_deaths +
Africa 2024-08-19 49 48
Africa 2024-08-20 49 48
Africa 2024-08-21 49 48
Africa 2024-08-24 49 48
Africa 2024-08-25 49 48
~ Column total_deaths_per_million (changed data)
- - Removed values: 43 / 85208 (0.05%)
country date total_deaths_per_million
Cote d'Ivoire 2024-07-11 0.0
Cote d'Ivoire 2024-07-17 0.0
Cote d'Ivoire 2024-08-03 0.0
Cote d'Ivoire 2024-08-06 0.0
Cote d'Ivoire 2024-08-12 0.0
~ Changed values: 8 / 85208 (0.01%)
country date total_deaths_per_million - total_deaths_per_million +
Africa 2024-08-19 0.03267 0.03201
Africa 2024-08-20 0.03267 0.03200
Africa 2024-08-21 0.03267 0.03200
Africa 2024-08-24 0.03266 0.03200
Africa 2024-08-25 0.03266 0.03199
Legend: +New ~Modified -Removed =Identical Details
Hint: Run this locally with etl diff REMOTE data/ --include yourdataset --verbose --snippetAutomatically updated datasets matching weekly_wildfires|excess_mortality|covid|fluid|flunet|country_profile|garden/ihme_gbd/2019/gbd_risk are not included Edited: 2024-08-27 09:35:30 UTC |
Marigold
force-pushed
the
revert-step-update
branch
from
d354097
to
e693dc9
Compare
Marigold
requested review from
lucasrodes and
pabloarosado
and removed request for
lucasrodes
docs/guides/data-work/update-data.md
Outdated
| Run the command you copied from the ETL Dashboard: | ||
|
|
||
| ```bash | ||
| etl pr update-{short_name} --title ":bar_chart: Update {short_name}" --step-update | ||
| etl pr update-{short_name} --title ":bar_chart: Update {short_name}" |
There was a problem hiding this comment.
I was confused by this line.
I think ETL Dashboard is not displaying this command anywhere. I think it might be being printed in the terminal?
I think it'd be great If ETL Dashboard displayed it. cc. @pabloarosado
There was a problem hiding this comment.
Good point. It's displayed in the terminal which is a bit hidden. I removed the mention of ETL dashboard and only left the command.
docs/guides/data-work/update-data.md
Outdated
|
|
||
| Then, commit generated files and push them to the new branch. This commit will contain only copied steps, subsequent changes | ||
| will be added as separate commits. | ||
|
|
There was a problem hiding this comment.
This can lead to an errors in the CI/CD. Take my example, where CI/CD was failing bc I hadn't executed the snapshot steps before pushing.
I think it is natural that we sometimes won't be able to run the Snapshot steps before pushing. We might just want to save our workspace, before even looking at the snapshot files. Even, some of them might not run anymore (e.g. changes in the download link, etc.).
I think we need a warning (expect CI/CD to fail if you haven't run snapshot or data steps) and a calming message (this will be fixed as you run and edit the steps).
There was a problem hiding this comment.
Updated the comment with a warning. You should get a decent error now telling you that you should probably run the snapshot.
There was a problem hiding this comment.
Thanks for the improvements @Marigold, I think that some of the stuff you did here were very useful, but the main issue is automatically adding and committing everything. Maybe we could simply ask for confirmation (instead of reverting everything). What do you think?
| @@ -224,19 +213,6 @@ def cli( | |||
| log.error(f"Failed to create draft pull request:\n{response.json()}") | |||
| return | |||
|
|
|||
| # modify the PR body to include the diff link | |||
| if step_update: | |||
| diff_link = f"{js['html_url']}/files/{repo.head.commit.hexsha}..HEAD" | |||
There was a problem hiding this comment.
I think this was quite useful, I'm sad to see this go!
There was a problem hiding this comment.
I thought that too, but now I think it's more intuitive to just copy the link from the docs or select correct commits if you're a reviewer. I'm sure we'll find a better way to display this in the future.
| @@ -199,11 +193,6 @@ def cli( | |||
| log.info("Creating an empty commit.") | |||
| repo.git.commit("--allow-empty", "-m", title or f"Start a new staging server for branch '{new_branch}'") | |||
|
|
|||
| if step_update: | |||
| log.info("Committing all files.") | |||
There was a problem hiding this comment.
What if we simply print the list of files to be commited, and ask for confirmation, before adding and committing?
There was a problem hiding this comment.
If there are snapshot files in the list, the printed message should mention that snapshot scripts would need to be executed prior to committing (or otherwise expect CI to fail, as suggested by @lucasrodes).
There was a problem hiding this comment.
Feel free to try my suggestions, or to simply go ahead and merge this PR as it is (with Lucas' suggestion about snapshots), if you don't feel like spending more time on these details. Either option is ok but not ideal in any case. Sorry for the back and forth.
I've actually changed my mind after our discussion. It's nicer to have ETL dashboard and git operations separated. It's just two extra commands that the user has to run, and it's way more transparent. If we decide to integrate them together, we'd better do it all in ETL dashboard without having to run CLI. |
Revert
etl pr ... --step-updateand update ETL docs. User has to manually commit generated files now and insert the diff link in the description.