Repository navigation
Make cache keys the same whatever the order and letter case of the Evidence keys - #125
Merged
Merged
Conversation
… the letters of its keys
…d in order, once before processing
# Conflicts: # pipeline/fiftyone.pipeline.engines/types/engine.d.ts
jwrosewell
marked this pull request as ready for review
November 15, 2026 00:33
This was referenced Nov 15, 2026
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.jslines 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 asheader.User-Agentwhere an earlier request sentheader.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.mdlines 66 to 68 at 706de1f0:From line 71 of the same page:
From line 82 of the same page:
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.processmakes 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 toinCache, which makes one itself only when it is called without one.types/engine.d.tsdeclares the new method and the new optional argument ofinCache.Tests
The new
pipeline/fiftyone.pipeline.engines/tests/cacheKey.test.jsuses 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 thatrcfails 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
rcbelow 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 warningsrcalready has, inpipeline.jsandparametersWithoutEndpoint.test.js.The same file checks that each request makes its cache key once, counting the calls to
cacheKeyover 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 tscon this head and found no difference in the committed types butpipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, which #127 decides.Full build
Run 43654102474 on
tmp/pr-matrix-125-c, started bytools/pr-matrix.pywithout macOS. All 7Setting '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
rcafter this draft was first built and wrotetypes/engine.d.tsagain, so mergingrc(0f033e3) conflicted in that file, as this section said it would. The merge keepsrc's file and putscacheKeybetweenisCacheableandinCache, which is where tsc writes it, as the types run above shows.What remains
Nothing for these requirements.