Skip to content

Answer then on a Flow Data without looking for Element Data - #175

Draft
jwrosewell wants to merge 2 commits into
rcfrom
fix/flowdata-then
Draft

jwrosewell wants to merge 2 commits into
rcfrom
fix/flowdata-then

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 15, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A promise reads then from every value it resolves with, to see whether that value is itself a promise. The Pipeline resolves a promise with the Flow Data before its first element and after each one (pipeline/fiftyone.pipeline.core/pipeline.js lines 164, 176, 191, 210 and 221 at c6ba938), and the caller's await flowData.process() does it once more.

The Flow Data is a proxy that passed each read of then to get (pipeline/fiftyone.pipeline.core/flowData.js lines 64 to 89). get (lines 176 to 197) logs the read at debug level, finds no element called then, looks again ignoring letter case, builds a message naming every key and throws it, and the proxy caught that and gave undefined. So every request ran the lookup for a missing key several times over. Looking again ignoring letter case came with #122, which made each of those misses dearer.

Timed against main while rc was checked for release, the IP Intelligence on-premise engine gave 5.8 to 8.0 per cent fewer detections a second in five runs on rc commits from 6c0b3fa to c6ba938. An earlier timing had found the loss between 7004cc6 (run 43696340276, 0.9 per cent fewer) and cdbbee8 (run 43696348092, 6.0 per cent fewer). Timing each of the five commits between them put the step at #122, with the commit before it giving 1.2 per cent more than main and #122's own commit 5.7 per cent fewer, as the table in Performance shows.

What changed

  • The proxy answers then as undefined before any lookup, because then is never Element Data (flowData.js lines 71 to 78 at b337ec1). The catch that existed only to swallow the error for then is removed, so every other key behaves as before. It gives Element Data, a method or property of the Flow Data, or the error for a key no element has.

fiftyone.pipeline.core changes, so it is released, with the packages that depend on it.

Before you upgrade

Nothing changes for existing code. Reading then from a Flow Data gave undefined before, through the caught error, and gives undefined now. An element whose data key is then is no longer found as flowData.then, which a promise would have taken for a then method anyway, and flowData.get('then') still finds it.

Tests

A new pipeline/fiftyone.pipeline.core/tests/flowDataThen.test.js builds a Pipeline of two elements and watches FlowData.get.

  1. Reading then gives undefined and calls get not at all.
  2. Processing the Flow Data and awaiting it never calls get for then.
  3. Element Data is still found by key, as first and as SECOND, through get.
  • Run 43751293582, the red run, on 4278de7, which is rc at c6ba938 with only the test file, 0f5dd4b, and a throwaway workflow added. The first two tests fail and the third passes, as it should before and after the change. The other 584 core unit tests pass.
  • Run 43751302403, the green run, on 97e3ff7, which is this head, b337ec1, with only the same throwaway workflow added. All 586 core unit tests pass, and lint passes on flowData.js and the test file.

Performance

Each run below is a throwaway workflow on one Ubuntu ARM runner with Node.js 24. It times main at 4d5832f against one other commit with the on-premise performance console examples, Device Detection on the TAC data file and IP Intelligence on the Enterprise data file, both taken from the repository's asset cache. Each run has 21 rounds, turning the order each round so that neither goes first every time, and reads HigherIsBetter.DetectionsPerSecond from each example's JSON output. Figures are medians in detections a second, higher is better, with the number of rounds in which the other commit beat main.

Run Commit Device Detection, commit against main IP Intelligence, commit against main
43749983846 0effb87, before #122 34,338 against 35,560, -3.4 per cent 35,661 against 35,255, +1.2 per cent
43749995477 51d9a3f, #122 33,597 against 35,027, -4.1 per cent 33,521 against 35,537, -5.7 per cent
43747756367 c6ba938, rc 30,753 against 33,846, -9.1 per cent, 6 of 21 31,869 against 33,977, -6.2 per cent, 5 of 21
43748812765 c6ba938, rc, again 32,675 against 34,466, -5.2 per cent, 5 of 21 32,446 against 34,701, -6.5 per cent, 4 of 21
43751311143 b337ec1, this head 42,830 against 33,378, +28.3 per cent, 20 of 21 42,938 against 34,097, +25.9 per cent, 16 of 21

main runs the same miss for then without the second look, so this head gives more detections a second than main as well as more than rc. In the last run Device Detection's lowest round on this head, 40,525, is below main's highest, 42,644, and the same holds for IP Intelligence, 40,507 against 43,980, so the full ranges touch. The quartiles are apart, at 41,680 to 46,823 against 33,123 to 34,075 for Device Detection and 41,914 to 43,901 against 33,367 to 35,455 for IP Intelligence.

Full build

Run 43752383786 on tmp/pr-matrix-175, started by tools/pr-matrix.py without macOS. All 7 Setting 'PullRequestSha' lines give this head, b337ec1, 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,949 unit tests with 2 skipped and 124 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.

What remains

Nothing for this change.

…led then

The Pipeline resolves a promise with the Flow Data at each of its
elements, and a promise reads then from what it resolves with. The Flow
Data passes that read to get, which finds no element, looks again ignoring
case and throws, on every request.
A promise reads then from every value it resolves with, and the Pipeline
resolves one with the Flow Data at each element. The proxy passed that
read to get, which since the case-ignoring lookup also scans every key
before it throws, several times a request. Timed against main, that
lookup cost the IP Intelligence on-premise engine about 6 per cent of
its detections a second. then is never Element Data, so the proxy now
answers it as undefined first, and the catch that only swallowed the
error for then goes.
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