Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web): test skipped prediction round handling #12169

Merged
merged 2 commits into from
Aug 14, 2024
Merged

feat(web): test skipped prediction round handling #12169

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0055ad8
feat(web): test skipped prediction round handling
jahorton Aug 13, 2024
7ab7ec9
chore(web): minor comment cleanup
jahorton Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,65 @@ describe("PredictionContext", () => {
assert.equal(suggestions.find((obj) => obj.transform.deleteLeft != 0).displayAs, 'apps');
});

it('ignores outdated predictions', async function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare and contrast with the test defined immediately before this one.

const langProcessor = new LanguageProcessor(worker, new TranscriptionCache());
await langProcessor.loadModel(appleDummyModel); // await: must fully 'configure', load script into worker.

const kbdProcessor = new KeyboardProcessor(deviceSpec);
const predictiveContext = new PredictionContext(langProcessor, kbdProcessor);

let updateFake = sinon.fake();
predictiveContext.on('update', updateFake);

let mock = new Mock("appl", 4); // "appl|", with '|' as the caret position.
const initialMock = Mock.from(mock);
const promise = predictiveContext.setCurrentTarget(mock);

// Initial predictive state: no suggestions. context.initializeState() has not yet been called.
assert.equal(updateFake.callCount, 1);
assert.isEmpty(updateFake.firstCall.args[0]); // should have no suggestions. (if convenient for testing)

await promise;
let suggestions;

// Initialization results: our first set of dummy suggestions.
assert.equal(updateFake.callCount, 2);
suggestions = updateFake.secondCall.args[0];
assert.deepEqual(suggestions.map((obj) => obj.displayAs), ['apple', 'apply', 'apples']);
assert.isNotOk(suggestions.find((obj) => obj.tag == 'keep'));
assert.isNotOk(suggestions.find((obj) => obj.transform.deleteLeft != 0));

const baseTranscription = mock.buildTranscriptionFrom(initialMock, null, true);

// Mocking: corresponds to the second set of mocked predictions - round 2 of
// 'apple', 'apply', 'apples'.
const skippedPromise = langProcessor.predict(baseTranscription, kbdProcessor.layerId);
Copy link
Contributor Author

@jahorton jahorton Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, I didn't end up using this value. Not sure what the best way to test with it would be, so maybe I should just...

Suggested change
const skippedPromise = langProcessor.predict(baseTranscription, kbdProcessor.layerId);
langProcessor.predict(baseTranscription, kbdProcessor.layerId);

and disregard its returned value completely... which is what happens in production, anyway.

This spot does have a notable contrast with the (previous) test that it's based upon, so I'll leave it in for the moment in case the distinction helps with the code review.


mock.insertTextBeforeCaret('e'); // appl| + e = apple
const finalTranscription = mock.buildTranscriptionFrom(initialMock, null, true);

// Mocking: corresponds to the third set of mocked predictions - 'applied'.
const expectedPromise = langProcessor.predict(finalTranscription, kbdProcessor.layerId);

await Promise.all([skippedPromise, expectedPromise]);
const expected = await expectedPromise;

// Despite two predict calls, we should only increase the counter by ONE - we ignore
// the 'outdated' / 'skipped' round because it could not respond before its followup.
assert.equal(updateFake.callCount, 3);
suggestions = updateFake.thirdCall.args[0];

// This does re-use the apply-revert oriented mocking.
// Should skip the (second) "apple", "apply", "apps" round, as it became outdated
// by its following request before its response could be received.
assert.deepEqual(suggestions.map((obj) => obj.displayAs), ['“apple”', 'applied']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we get a different set of suggestions than in the previous test? Ok, the difference is the additional predict call, but why/how would that get triggered in production?

Copy link
Contributor Author

@jahorton jahorton Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why/how would that get triggered in production?

  • Suppose I quickly double-tap a key on the OSK. Prediction may be on the order of tens of milliseconds, but it still takes time.
  • Or that I am using multiple fingers to type and type two keys near-simultaneously - especially if using a hardware keyboard (which we do support on Android)
  • Or that I am using an older Android device where we previously had rapid-typing bugs due to a lower-end CPU on the device - the predictive-text processing would likely also be slow in such a scenario, with both competing for CPU cycles.
    • Of particular note - the JS task queue and microtask queue tend to be FIFO in my experience, and the implications of this are significant here. (Just as they were for resolving the rapid-typing bugs from 17.0-beta.)
    • All pending keys to be processed would each trigger their own predict call when processed, each one likely cancelling the previous one.
      • Even if the worker does get processing time in parallel, the worker's response would only fulfill the prediction Promise once the suggestions are actually ready - a time much later than the triggering key.
      • Thus, the response's Promise fulfillment would be queued after fulfillment for Promises for the pending keys - which could even include keys queued since the original predict() call!
      • This, in turn, would cause PredictionContext to skip the predictions - which is a good thing here. We don't want to rapidly rotate through numerous invalid suggestions while we're already lagging - updating the DOM does take up CPU time, after all. A smoother suggestion-bar flow would also likely be preferable to users (over one rapidly changing while not meaningfully interactive)

That list isn't necessarily exhaustive, but highlights the most common cases I'd expect to trigger this.

I got a bit curious what the average delay between keystrokes might be. It's naturally going to be subject to variability, but one paper states that, for PIN input on touchscreen devices, roughly 270 ms is the average delay between keystrokes... but with a nearly equal standard deviation of 267ms. Note the paper's "Figure 3", which has a graph of their data for this.

Based on the graph data, it's certainly not rare to have too short an amount of time between keystrokes for a prediction-search to use all of its allotted time. If both happen at the same time, we'd trigger a new predict request before the first completes, triggering such a "skip".

assert.equal(suggestions.find((obj) => obj.tag == 'keep').displayAs, '“apple”');
assert.equal(suggestions.find((obj) => obj.transform.deleteLeft != 0).displayAs, 'applied');
// Our reused mocking doesn't directly provide the 'keep' suggestion; we
// need to remove it before testing for set equality.
assert.deepEqual(suggestions.splice(1), expected);
});

it('sendUpdateState retrieves the most recent suggestion set', async function() {
const langProcessor = new LanguageProcessor(worker, new TranscriptionCache());
await langProcessor.loadModel(appleDummyModel); // await: must fully 'configure', load script into worker.
Expand Down
Loading