Skip to content

Make cache keys the same whatever the order and letter case of the Evidence keys - #125

Merged
jwrosewell merged 5 commits into
rcfrom
fix/cache-keys
Nov 15, 2026
Merged

jwrosewell merged 5 commits into
rcfrom
fix/cache-keys

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

An Engine's cache key is the JSON of the Evidence it accepts, in the order the Evidence was added and with each key as it was first spelled (pipeline/fiftyone.pipeline.engines/engine.js lines 99 to 104 and 136 to 141 at 6d5d4fef). So the same Evidence added in another order, or with a key in other letters, such as header.User-Agent where an earlier request sent header.user-agent, misses the cache and is processed again. The key stored was also made after processing, from Evidence processing may have added to, where the lookup is made before.

What the specification says

From pipeline-specification/features/caching.md lines 66 to 68 at 706de1f0:

Cache keys MUST always be generated deterministically. This means that the order that Evidence values are considered must be the same each time.

From line 71 of the same page:

Comparison of Evidence keys MUST be case-insensitive.

From line 82 of the same page:

Comparison of Evidence values MUST be case-sensitive.

What changed

A new Engine.cacheKey(flowData) makes the key from the Evidence the Engine accepts, with each key in lower case, sorted by key, and each value as it was given. process makes the key once, before processing, so a result is stored under the key the next request with the same Evidence looks for, and passes it to inCache, which makes one itself only when it is called without one. types/engine.d.ts declares the new method and the new optional argument of inCache.

Tests

The new pipeline/fiftyone.pipeline.engines/tests/cacheKey.test.js uses one Engine that has a cache and counts what it processes. It checks that the same Evidence added in another order is served from the cache, that a key in other letters is served from the cache, and that a value in other letters is not.

Red, run 43637356487 on 7a58bd4, the test commit on rc's code, with only a throwaway workflow added, running every test of the pipeline packages but the integration tests and the OWID submodule's own. On Ubuntu and on Windows exactly the two new tests that rc fails fail, "the same Evidence added in another order is served from the cache" and "an Evidence key in other letters is served from the cache", and the other 1,186 pass.

Green, run 43637366748 on 0272437, the fix before the merge of rc below and before a0c1e0b, with only the same throwaway workflow added. All 1,188 pass on Ubuntu and on Windows. Lint passes there with the two warnings rc already has, in pipeline.js and parametersWithoutEndpoint.test.js.

The same file checks that each request makes its cache key once, counting the calls to cacheKey over one request that is processed and one served from the cache.

Red, run 43653866595 on c4fefb3, that test on this draft's code before a0c1e0b, with only a throwaway workflow added that runs the engines package's tests. On Ubuntu and on Windows that test fails, as each request made its key twice, and the other 93 pass.

Green, run 43653873761 on a0c1e0b, this head, with only the same throwaway workflow added. All 94 pass on Ubuntu and on Windows, and lint passes on the package.

Types, run 43654077878 ran npm run tsc on this head and found no difference in the committed types but pipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, which #127 decides.

Full build

Run 43654102474 on tmp/pr-matrix-125-c, started by tools/pr-matrix.py without macOS. All 7 Setting 'PullRequestSha' lines give this head, a0c1e0b, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,516 unit tests with 2 skipped and 96 integration tests with 4 skipped.

macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.

The merge of rc

#113 merged into rc after this draft was first built and wrote types/engine.d.ts again, so merging rc (0f033e3) conflicted in that file, as this section said it would. The merge keeps rc's file and puts cacheKey between isCacheable and inCache, which is where tsc writes it, as the types run above shows.

What remains

Nothing for these requirements.

@jwrosewell
jwrosewell marked this pull request as ready for review November 15, 2026 00:33
@jwrosewell
jwrosewell merged commit 319aa7e into rc Nov 15, 2026
2 checks passed
@jwrosewell
jwrosewell deleted the fix/cache-keys branch November 15, 2026 00:33
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.

1 participant