Fix multidimensional data pages baking logic #4478

rakyi · 2025-01-23T10:55:16Z

Don't delete mdims in GrapherBaker
Delete old mdims in MultiDimBaker
Improve the console output
Document issue with TSConfig and getting ESNext types

rakyi · 2025-01-23T10:58:29Z

baker/GrapherBaker.tsx

@@ -308,7 +309,6 @@ const bakeGrapherPageAndVariablesPngAndSVGIfChanged = async (
            imageMetadataDictionary
        )
    )
-    console.log(outPath)


We output the slug in the progress bar already, this doesn't add anything useful on top of that.

rakyi · 2025-01-23T11:00:08Z

baker/GrapherBaker.tsx

@@ -432,7 +431,7 @@ export const bakeAllChangedGrapherPagesVariablesPngSvgAndDeleteRemovedGraphers =
        )

        const progressBar = new ProgressBar(
-            "bake grapher page [:bar] :current/:total :elapseds :rate/s :etas :name\n",


It's not clear this is an ETA in the output, which is already cluttered, and doesn't seem useful.

rakyi · 2025-01-23T11:00:35Z

baker/GrapherBaker.tsx

@@ -451,11 +450,18 @@ export const bakeAllChangedGrapherPagesVariablesPngSvgAndDeleteRemovedGraphers =
                    async (knex) => await bakeSingleGrapherChart(job, knex),
                    db.TransactionCloseMode.KeepOpen
                )
-                progressBar.tick({ name: `slug ${job.slug}` })
+                progressBar.tick({ name: job.slug })


Repeating slug on every line doesn't add anything useful.

owidbot · 2025-01-23T11:44:17Z

Quick links (staging server):

Site Dev	Site Preview	Admin	Wizard	Docs

Login: ssh owid@staging-site-fix-mdim-baking-logic

Site-screenshots: https://github.com/owid/site-screenshots/compare/fix-mdim-baking-logic
SVG tester: https://github.com/owid/owid-grapher-svgs/compare/fix-mdim-baking-logic

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2025-01-23 11:44:17 UTC
Execution time: 1.23 seconds

mlbrgl

Nice!

My main concern is about the repetition of deleteOldGrapher / deleteOldMultiDimPages:
1- code redundancy: they largely do the same thing
2- execution redundancy: the deletion of obsolete /grapher pages happens twice (once at the grapher step, and another time at the mdim step)

2 is more of an optimzation for full bakes, and doesn't probably count for much so I wouldn't necessarily complexify here. Also it's nice that each step is self-contained when we do partial bakes.

The near duplication of deleteOldGrapher in 1 is a bit more cumbersome, and it would be more straightforward to have a single function that deals with the overall deletion of /grapher pages (even if it called twice, as per 2).

It would be also a good opportunity to deal with the confusion around redirects to explorers.

On the one hand, we don't delete grapher pages that have been redirected to explorers:

owid-grapher/baker/GrapherBaker.tsx

Lines 344 to 346 in 2d02649

    
           const toRemove = without(oldSlugs, ...newSlugs) 
        
               // do not delete grapher slugs redirected to explorers 
        
               .filter((slug) => !isPathRedirectedToExplorer(`/grapher/${slug}`))

On the other hand, we don't bake grapher pages that have been redirected to explorers:

owid-grapher/baker/GrapherBaker.tsx

Lines 377 to 382 in 2d02649

    
           // Avoid baking paths that have an Explorer redirect. 
        
           // Redirects take precedence. 
        
           if (isPathRedirectedToExplorer(`/grapher/${grapher.slug}`)) { 
        
               console.log(`⏩ ${grapher.slug} redirects to explorer`) 
        
               return 
        
           }

Meanwhile, deleteOldMultiDimPages does delete grapher pages that have been redirected to explorers, and effectively overrides deleteOldGrapher's safeguard. If this safeguard is no longer relevant, I'd remove it rather than having it in one place and not the other, which is confusing.

mlbrgl · 2025-01-24T17:20:08Z

baker/MultiDimBaker.tsx

+        console.log(`DELETING ${slug}`)
+        const path = `${bakedSiteDir}/grapher/${slug}.html`
+        await fs.unlink(path)
+        console.log(path)


console.log(path) necessary? We have the same console.log in deleteOldGrapher().

Also, instead of:

DELETING coronavirus-cfr localBake/grapher/coronavirus-cfr.html

we could maybe simply output:

DELETING localBake/grapher/coronavirus-cfr.html

?

I did it that way to be consistent with deleteOldGraphers, but that has since been changed in master, so I'm simplifying it.

mlbrgl · 2025-01-24T18:18:02Z

baker/MultiDimBaker.tsx

    }
+    const publishedSlugs = await getAllPublishedMultiDimDataPageSlugs(knex)


you could get the list of published slugs from multiDimsBySlug potentially, to avoid the extra db query?

Yep. I started with that, then changed the logic a few times and then forgot that was an option. 😄

rakyi · 2025-01-27T10:47:30Z

Good point, I moved the deletion of old graphers to a single helper function called in both places. After the removal of image exports, there is no longer a distinction between normal graphers and mdims. I kept the safeguard for redirects to explorers just in case.

* Don't delete mdims in GrapherBaker * Delete old mdims in MultiDimBaker * Improve the console output * Document issue with TSConfig and getting ESNext types

mlbrgl

Looks good, thanks for the updates!

rakyi requested a review from mlbrgl January 23, 2025 10:55

github-actions bot assigned rakyi Jan 23, 2025

rakyi commented Jan 23, 2025

View reviewed changes

rakyi force-pushed the fix-mdim-baking-logic branch from 6e05aa2 to 8385cc3 Compare January 23, 2025 11:29

rakyi force-pushed the fix-mdim-baking-logic branch from 8385cc3 to 2a6a3d2 Compare January 23, 2025 12:31

rakyi linked an issue Jan 23, 2025 that may be closed by this pull request

Correctly delete unpublished multidimensional data pages when baking #4482

Closed

rakyi mentioned this pull request Jan 23, 2025

Update base TSConfig to options supported by Node 22 #4481

Merged

rakyi force-pushed the fix-mdim-baking-logic branch 4 times, most recently from af22b53 to 2d02649 Compare January 24, 2025 10:01

mlbrgl reviewed Jan 24, 2025

View reviewed changes

rakyi force-pushed the fix-mdim-baking-logic branch 2 times, most recently from 5d083fe to 92fdd3e Compare January 27, 2025 10:49

Fix multidimensional data pages baking logic

f235b87

* Don't delete mdims in GrapherBaker * Delete old mdims in MultiDimBaker * Improve the console output * Document issue with TSConfig and getting ESNext types

rakyi force-pushed the fix-mdim-baking-logic branch from 92fdd3e to f235b87 Compare January 27, 2025 10:54

rakyi requested a review from mlbrgl January 27, 2025 10:55

rakyi mentioned this pull request Jan 28, 2025

Implement thumbnails for multidimensional data pages #4475

Merged

mlbrgl approved these changes Jan 29, 2025

View reviewed changes

rakyi merged commit d3760e4 into master Jan 29, 2025
27 checks passed

rakyi deleted the fix-mdim-baking-logic branch January 29, 2025 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multidimensional data pages baking logic #4478

Fix multidimensional data pages baking logic #4478

rakyi commented Jan 23, 2025 •

edited

Loading

rakyi Jan 23, 2025

rakyi Jan 23, 2025

rakyi Jan 23, 2025

owidbot commented Jan 23, 2025

mlbrgl left a comment

mlbrgl Jan 24, 2025

rakyi Jan 27, 2025

mlbrgl Jan 24, 2025

rakyi Jan 27, 2025

rakyi commented Jan 27, 2025 •

edited

Loading

mlbrgl left a comment

	const toRemove = without(oldSlugs, ...newSlugs)
	// do not delete grapher slugs redirected to explorers
	.filter((slug) => !isPathRedirectedToExplorer(`/grapher/${slug}`))

	// Avoid baking paths that have an Explorer redirect.
	// Redirects take precedence.
	if (isPathRedirectedToExplorer(`/grapher/${grapher.slug}`)) {
	console.log(`⏩ ${grapher.slug} redirects to explorer`)
	return
	}

		}
		const publishedSlugs = await getAllPublishedMultiDimDataPageSlugs(knex)

Fix multidimensional data pages baking logic #4478

Fix multidimensional data pages baking logic #4478

Conversation

rakyi commented Jan 23, 2025 • edited Loading

rakyi Jan 23, 2025

Choose a reason for hiding this comment

rakyi Jan 23, 2025

Choose a reason for hiding this comment

rakyi Jan 23, 2025

Choose a reason for hiding this comment

owidbot commented Jan 23, 2025

mlbrgl left a comment

Choose a reason for hiding this comment

mlbrgl Jan 24, 2025

Choose a reason for hiding this comment

rakyi Jan 27, 2025

Choose a reason for hiding this comment

mlbrgl Jan 24, 2025

Choose a reason for hiding this comment

rakyi Jan 27, 2025

Choose a reason for hiding this comment

rakyi commented Jan 27, 2025 • edited Loading

mlbrgl left a comment

Choose a reason for hiding this comment

rakyi commented Jan 23, 2025 •

edited

Loading

rakyi commented Jan 27, 2025 •

edited

Loading