1681985 - Use Fathom to recognize CC fields

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P2)

Description

[Erik Rose [:erik][:erikrose]]

Enlist a Fathom-powered model to recognize credit card fields, delivering higher accuracy than the current heuristics.

Comment 1

[Erik Rose [:erik][:erikrose]]

Attachment: Bug 1681985 - Extract LabelUtils to FormAutofillUtils.jsm. r=zbraniecki

We need it from both FormAutofillHeuristics and CreditCardRuleset, and it would make a circular import otherwise: FormAutofillHeuristics -> CreditCardRuleset -> FormAutofillHeuristics.

Comment 2

[Erik Rose [:erik][:erikrose]]

Attachment: Bug 1681985 - Add and call Fathom ruleset. r=zbraniecki

Remove calls to old heuristics, which are duplicated, improved, expanded, and balanced in the Fathom model. We'll come along in the next release and delete the dead code. We're leaving it in for now to keep it from bitrotting, in case we have to revert to it.

Depends on D100140

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b515a3ab8bac
Extract LabelUtils to FormAutofillUtils.jsm. r=zbraniecki
https://hg.mozilla.org/integration/autoland/rev/1ba9b39ff543
Add and call Fathom ruleset. r=zbraniecki

Comment 4

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=AowlaU2LQzOY1BA6ADCFDA.0&revision=1ba9b39ff543cf15ed03f54a744c935695588431

Logs:
https://treeherder.mozilla.org/logviewer?job_id=325319721&repo=autoland&lineNumber=3510
https://treeherder.mozilla.org/logviewer?job_id=325318743&repo=autoland&lineNumber=1947

Backout: https://hg.mozilla.org/integration/autoland/rev/8e4cb67cacd9f49e5aabb599b9d329a7d06e6bca

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:erik, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 6

[Erik Rose [:erik][:erikrose]]

./mach try auto didn't reveal the test failures that finally happened in the full run. I'm chasing the failures now.

Comment 7

[Erik Rose [:erik][:erikrose]]

As of the current patch, here are the pre- and post-Fathom testing-set metrics. There's a dip on expiration, but, on average, there's about a 5% boost in both precision and recall across rulesets.

cardType
Testing precision: 1.0000   Recall: 1.0000
Pre-Fathom testing precision: 1.0000   Recall: 0.8182

name
Testing precision: 1.0000   Recall: 0.9231
Pre-Fathom testing precision: 0.8571   Recall: 0.9231

number
Testing precision: 0.9643   Recall: 0.9643
Pre-Fathom testing precision: 0.7778   Recall: 1.0000

expiration
Testing precision: 1.0000   Recall: 0.7500
Pre-Fathom testing precision: 1.0000   Recall: 0.8750

expirationMonth
Testing precision: 1.0000   Recall: 0.8800
Pre-Fathom testing precision: 1.0000   Recall: 0.7391

expirationYear
Testing precision: 1.0000   Recall: 0.9167
Pre-Fathom testing precision: 1.0000   Recall: 0.7727

Comment 8

[Erik Rose [:erik][:erikrose]]

Attachment: Bug 1681985 - Update Fathom to 3.7.3.

Get a version which will throw a specific error when isVisible() is run on elements that aren't in a window.

Some of the xpcshell tests crash because they transit through the codepath that calls Fathom, even though they don't do anything with its output. Fathom gets cranky and throws an exception because the elements it's evaluating don't live within a window object (an artifact of the test harness). This lets us swallow that exception and no others.

Depends on D100140

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/386a8b11c127
Extract LabelUtils to FormAutofillUtils.jsm. r=zbraniecki
https://hg.mozilla.org/integration/autoland/rev/e77582268ce1
Update Fathom to 3.7.3. r=zbraniecki
https://hg.mozilla.org/integration/autoland/rev/7b19f4ed5182
Add and call Fathom ruleset. r=zbraniecki

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/386a8b11c127
https://hg.mozilla.org/mozilla-central/rev/e77582268ce1
https://hg.mozilla.org/mozilla-central/rev/7b19f4ed5182

Comment 11

[Pascal Chevrel:pascalc]

Erik, this enhancement already caused 3 known regressions in the past week, are you working on them or should we backout these patches? Thanks

Comment 12

[Alexandru Michis [:malexandru]]

Backed out 3 changesets (Bug 1681985) for causing performance issues.
Backout link: https://hg.mozilla.org/integration/autoland/rev/912ebd9e294c795077e75ef7a849d7d854a02a53

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/912ebd9e294c

Comment 14

[Alex Thayer [:alexical] (she/her)]

Attachment: WIP: Bug 1681985 - Implement CreditCardRuleset in C++

Comment 15

[Alex Thayer [:alexical] (she/her)]

I've attached a functional patch for a version of this which is able to handle 1000 input fields with an overhead of about 0.02s. Contrast this with about 30s from the old implementation (I believe that clocks this in as a 1500x perf improvement?). I think this is sufficiently performant for us to not have a threshold at which we bail out, and it means that the vast majority of the jank we see on pages with large numbers of inputs will come from preexisting code.

It still needs a bit of work, and I need to find a reviewer for it (Zibi?). I think I may need to nuke some tests like Erik's patch did? I'm not entirely sure of the story behind that.

I also need to figure out how to implement the few lookahead/lookbehind regexes that we were using. I used rust's regex implementation because it's substantially faster and calling into our JS regex stuff from C++ was awkward? I think I can implement the lookaheads and lookbehinds using two regexes, and it won't be exactly correct but it should be close enough.

Comment 16

[Dimi Lee [:dimi]]

The plan is to land this in Q4 this year.

Comment 17

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P1. Extract LabelUtils to FormAutofillUtils.jsm.

Comment 18

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P2. Update Fathom to 3.7.3.

Depends on D137268

Comment 19

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P4. Add and call Fathom ruleset.

• We make some changes to heuristicsRegexp.js to improve accuracy. (Nothing else uses these regexps, so they're safe to change.) The commenting out of some languages in the expiration fields are because they caused a lot of false positives, according to Daniel Hertenstein's recollection. In any case, we've never preffed CC autofill on for those languages.

Depends on D137269

Comment 20

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P3. Implement CreditCardRuleset in C++

Depends on D137270

Comment 21

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P5. CredutCardRulset.jsm bug fixing

This patch aims to address the problems I found in Eric's patch (P3).
Create another revision for now to make review easier. When this is review+, this part should be merged to P3 of this patch.

This patch does the following:

1. Create ruleset for each "type"
2. Clear labelmap in getElementLabels API
3. Use a separate HeuristicsRegExp for fathom code. By doing this, we
   can support non-fathom heuristic and fathom heuristic at the same time.

Depends on D137271

Comment 22

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P6. Fix issues in "Implement CreditCardRuleset in C++"

This patch aims to address issues found in patch (P4).
Create another revision for now to make review easier. When this is review+, this part should be merged to P4 of this patch.

These issues are all disovered by running all samples in the
form-autofill-fathom repo and compare the confidence value.

Depends on D137272

Comment 23

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P5. Support calling fathom ruleset in both c++ and js

Support calling cc heuristic with 3 options:

1. Old regular expression matching heuristic
2. Fathom JS implementation
3. Fathom Native implementation

Depends on D137273

Comment 24

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P6. Add a testcase to verify the result of js and native fathom CC model

Depends on D137274

Comment 25

[Dimi Lee [:dimi]]

:tgiles, :sergey, some information for you to review these patches
P1 ~ P3 is almost the same as Erik's patch submitted begore (p1 & p2 are r+ by zibi previously).The only difference is that I removed some testcase changes made by Erik in P3. The reason is that for those testcase changes, I'll create a separate commit later.
P4 is :dthayer patch that uses C++ to implement the fathom rule sets

So what I really have done is from P5-P8.
P5 is to address issues I found in P3.
P6 is to address issues I found in P4.
P7 integrate fathom by adding a pref so we can choose which heuristic to run (default is still using the old heuristic).
P8 is testcases and the patch is still ongoing so I'm not asking review at this point.

Comment 26

[Dimi Lee [:dimi]]

Attachment: Bug 1681985 - P6. Fix issues in "Implement CreditCardRuleset in C++"

This patch aims to address issues found in patch (P4).
Create another revision for now to make review easier. When this is review+, this part should be merged to P4 of this patch.

These issues are all disovered by running all samples in the
form-autofill-fathom repo and compare the confidence value.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cba83f1ede00
P1. Extract LabelUtils to FormAutofillUtils.jsm. r=tgiles,sgalich
https://hg.mozilla.org/integration/autoland/rev/92744b52c7ff
P2. Update Fathom to 3.7.3. r=tgiles,sgalich
https://hg.mozilla.org/integration/autoland/rev/4f87d743d258
P3. Implement CreditCardRuleset in C++ r=sgalich,emilio
https://hg.mozilla.org/integration/autoland/rev/2a7059b1763c
P4. Add and call Fathom ruleset. r=sgalich
https://hg.mozilla.org/integration/autoland/rev/0d73f29cdb7d
P5. Support calling fathom ruleset in both c++ and js r=tgiles,sgalich
https://hg.mozilla.org/integration/autoland/rev/cecbdf7b47b9
P6. Add a testcase to verify the result of js and native fathom CC model r=tgiles,sgalich

Comment 28

[Chris Peterson [:cpeterson]]

Dimi, do you expect this Fathom change to fix some of the credit card autofill bugs under meta bug 1643006? Should I retest the credit card autofill bugs I filed for that meta bug?

Comment 29

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/cba83f1ede00
https://hg.mozilla.org/mozilla-central/rev/92744b52c7ff
https://hg.mozilla.org/mozilla-central/rev/4f87d743d258
https://hg.mozilla.org/mozilla-central/rev/2a7059b1763c
https://hg.mozilla.org/mozilla-central/rev/0d73f29cdb7d
https://hg.mozilla.org/mozilla-central/rev/cecbdf7b47b9

Comment 30

[Dimi Lee [:dimi]]

(In reply to Chris Peterson [:cpeterson] from comment #28)

Dimi, do you expect this Fathom change to fix some of the credit card autofill bugs under meta bug 1643006? Should I retest the credit card autofill bugs I filed for that meta bug?

Hi Chris, Thank you for offering the help!
This feature is still disabled by default because the current fathom cc model doesn't include samples from Germany and France. But even for US, I think we should include more samples for the training (We have around 500 samples for fathom new-password model but only 150 samples for the credit card). I plan to spent one more month to improve the model. I just file Bug 1759418 to enable this feature in Nightly. We can test those bugs once the bug is resolved.

Comment 31

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/mozilla-central/rev/cecbdf7b47b9

@Dimi, this commit includes 45 files as lfs, but I can't fetch the lfs objects for them. Do you have any suggestion? Thanks!

Comment 32

[Dimi Lee [:dimi]]

(In reply to Kai-Zhen Li [:seinlin][:kli] from comment #31)

https://hg.mozilla.org/mozilla-central/rev/cecbdf7b47b9

@Dimi, this commit includes 45 files as lfs, but I can't fetch the lfs objects for them. Do you have any suggestion? Thanks!

Hi Kai-Zhen,
Thank you for letting me know. I forgot my testing samples are lfs objects. I'll replace those with non-lfs files.