245499 - Need to alter junk plugin training data write policy and/or cancel Start/EndBatch calls

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Eyal Rozenberg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040602
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040602

Since the 'FIXED' resolution seems to be an incurable illness for bug 243680, I
am following timless' advice and filing a new bug.

The problem is this: The junk plugin's EndBatch() should be have to be called to
trigger a write of the training data to disk. There should be periodic checking
for dirtyness and writes to the disk. Particularly, training data should be
written to disk on MailNews shutdown. Also particularly, EndBatch() should not
have an unconditional writeTrainingData() call. In fact, it should be a good
idea to forego the use of StartBatch and EndBatch completely, since they seem to
serve no other function.

See also my comments in bug 243680 on this issue.

Note that this is _not_ an RFE, it is a bug. In fact, it is bug 243680, which
has simply lost its current trigger.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Jon Henry]

Confirming.

Comment 2

[Alexander Skwar]

Do I get this right? training.dat is still not updated correctly?

Comment 3

[Robert Kaiser]

Alexander:
Well, currently it to a certain part merely (if I understand correctly) is
updated correctly again, as there was a fix in a way that the real bug that is
described here is still in place, but not really triggered by the code currently.
With any small change it could be triggered again as how we are dealing with the
situation now is not completely logical for a coder that's not familiar with all
that.
I hope I summarized correctly what I was able to read in the other bug report.

Comment 4

[Eyal Rozenberg]

Attachment: a rough proposed fix

Here's a patch of what I have in mind. Comments are most welcome.

Comment 5

[Mike Cowperthwaite]

*** Bug 245644 has been marked as a duplicate of this bug. ***

Comment 6

[Eyal Rozenberg]

Attachment: proposed fix using prefs

now with preferences, and no more useless commented-out lines. Note that

-class Token;
+struct Token;

is to 'fix' a compiler warning: Token is redefined as a struct in the .cpp
file. This has nothing to do with this bug. Also rephrased some comments.

Comment 7

[Eyal Rozenberg]

Attachment: proposed fix v3

Neil is a bit busy right now, so I'll post the latest stop in our
rewrite-review cycle, for your viewing and commenting pleasure.

Comment 8

[Eyal Rozenberg]

Comment on attachment 150550 [details] [diff] [review]
proposed fix v3

Neil is still busy, so I'm making my first review request from someone I've
never talked to about the relevant bug. Let's hope this doesn't annoy him to
much.

Comment 9

[Eyal Rozenberg]

Comment on attachment 150550 [details] [diff] [review]
proposed fix v3

Neil is still busy, so I'm making my first review request from someone I've
never talked to about the relevant bug. Let's hope this doesn't annoy him to
much.

Comment 10

[Eyal Rozenberg]

steven.chapel: Yeah, maybe setting the 'blocking' flag would make someone
realize this bug exists. So far two people have ignored the review request for
over a month :-(

Comment 11

[Prognathous]

Eyal, I guess they both have a long queue. Check #mozilla for other Mail&News
reviewers who may have more free time (e.g. bienvenu, sspitzer...)

Prog.

Comment 12

[Eyal Rozenberg]

Attachment: proposed fix v4

diff against 1.8a3 (previous patch had a line replacement which had already
taken place since the diff was against 1.7 IIRC).

Comment 13

[Eyal Rozenberg]

Comment on attachment 159737 [details] [diff] [review]
proposed fix v4

David has done 2 sr's for me, so maybe he'll have time for an r as well. And
sspitzer as per prog's suggestion. Can't get any worse than no reply at all, I
guess...

Comment 14

[Robert Kaiser]

I haven't seen sspitzer reacting to review requests lately, I guess you might
want to ask bienvenu for both r/sr, as he can give sr as well (an let him decide
if he gives both or only one of r/sr), or use another combination of people who
do response to those requests...

Comment 15

[Prognathous]

(In reply to comment #14)
> I haven't seen sspitzer reacting to review requests lately,

He just sr'ed a patch yesterday -
https://bugzilla.mozilla.org/show_bug.cgi?id=259259#c8 , but I don't know if
Eyal should count on it though... ;-)

Prog.

Comment 16

[Worcester12345]

I see the blocking 1.8a4 was turned off. Should a target milestone of 1.9alpha
be added instead?

Is 1.8a4 ready soon? I am seeing a lot of blockers just turned off like this lately.

Comment 17

[Robert Kaiser]

Sasquatch:
If the patch gets review, super-review and approval for checkin to 1.8a4 in
time, it may still go into this release. Being marked blocking1.8a4- just means
that the release of this version won't be held up by having this bug not
resolved. It doesn't mean it can't be solved in this release.

Comment 18

[David :Bienvenu]

Comment on attachment 159737 [details] [diff] [review]
proposed fix v4

I'm ambivalent about this approach. I understand there are issues with the
current approach but there are also issues with this approach. - I'm also
unsure about the default 50 minute threshhold. We'd have to be darn sure that
we write out on exit, because a lot of users may not keep tbird running for 50
minutes (do we currently write out on exit?). There's also the issue of
crashing and losing data. Changing sr request to mscott for his input.

the code as is seems OK - to me, it's just a question of whether the approach
is better, and I'm ambivalent about that.

Comment 19

[Eyal Rozenberg]

David, remember that there is no loss of _existing_ data - the only thing one
stands to lose from not writing the training data is the effect of the spam from
the last < 50 minutes, which is rather insignificant - since if this is like the
rest of the spam you get you'll have other chances to learn from the spam, and
if it's unique you don't want to learn it anyway... :-)

Comment 20

[Worcester12345]

(In reply to comment #18)
...
> ...We'd have to be darn sure that
> we write out on exit, because a lot of users may not keep tbird running for 50
> minutes (do we currently write out on exit?). There's also the issue of
> crashing and losing data. ...


(In reply to comment #19)
> David, remember that there is no loss of _existing_ data - the only thing one
> stands to lose from not writing the training data is the effect of the spam from
> the last < 50 minutes, which is rather insignificant - since if this is like the

I'd say that this IS significant. I, and many others, open email, read a few,
reply, and get out in 10 minutes or less. Many spam get marked and moved in this
time, which is less than 50 minutes. No wonder the spam detection seems so poor.

Comment 21

[Eyal Rozenberg]

Just to make things clear: with the patch, training info _is_ written on exit.
The question is what happens when there's a crash.

Comment 22

[David :Bienvenu]

Comment on attachment 159737 [details] [diff] [review]
proposed fix v4

I'm thinking this is a good idea now, with the following caveat - I think we
should keep the start end batch capabilities in the interface, but the bayesian
filter plugin can ignore them - other plugins might find them useful. I'll put
an r=bienvenu on a patch that does that, if you're ok with that.

Comment 23

[David :Bienvenu]

the thing that pushed me off the fence about this approach is that when using
thunderbird on a flash memory device, writing the training set is expensive.

Comment 24

[Eyal Rozenberg]

I'm not sure I agree. First of all, which other plugins are we talking about? I
know that theoretically one can replace the bayesian plugin with something else
- but would another plugin conceivably want to know about batches?

A second question: is an information actually imparted when one tells the plugin
'start batch' or 'end batch'? The plugin knows when you're sending it messages
after not sending messages in a while, and it can also know (by using a timer)
when you haven't sent it messages in a while, i.e. it knows about batches
without anyone telling it about them.

I suspect the motivation for maintaining StartBatch/EndBatch is in fact the need
for an ability to tell the plugin "write your state data now" when you expect
something bad to happen. 

Comment 25

[David :Bienvenu]

Yes, the main point of begin batch, end batch is to give the plugin a chance to
flush its data to disk, but one could certainly imagine other uses. Re other
plugins, another scenario is that our existing plugin might become a lot smarter
about flushing minimal changes to disk, instead of rewriting the whole training
set - in that case, flushing the changes after getting an endBatch notification
might be reasonable. Re the timer, sure, that's an alternative, but I don't
think it's that difficult for us to send begin batch/end batch notifications.

Comment 26

[Eyal Rozenberg]

> I don't think it's that difficult for us to send begin batch/end batch 
> notifications.

It make us need to track when messages are being sent to the plugin, and it
makes us do it in a rather unintelligent way. If the user goes over a folder
message by message and marks single messages as junk, we'd be doing lots of
start batch / end batch calls for no reason. Like I said, the plugin can tell
when batches start and end without other parts of the code telling it to him.

So I still think it's better to leave out start/end batch, and have interface
methods for forcing the plugin to write its state data and for hinting for it to
write state data (all of it or part of it).

Comment 27

[David :Bienvenu]

Comment on attachment 159737 [details] [diff] [review]
proposed fix v4

ok, we can always add back the methods if it becomes an issue...but just so I
can get in the last word :-) we were trying to keep the start/end batch methods
 a little bit general, instead of specifically having a method that poked a
plugin to say "flush your training set".

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #19)
> the only thing one stands to lose from not writing the training data
> is the effect of the spam from the last < 50 minutes

This is not exactly correct, though it's not clear that this edge-case is
important enough to worry about: consider if someone marks a bunch of stuff as
spam, and then crashes.  Now, the headers of some set of messages are marked as
junk.  Suppose, then, that after restarting post-crash, the user un-marks them.
 Their token counts will be subtracted from the database totals, even though the
database never got written out to disk.  

Comment 29

[Eyal Rozenberg]

(In reply to comment #28)

My comment #19 is still correct, because the difference between the expected
state of the training data after the user's marking the messages as not junk and
the actual state is exactly the effect of their being marked as junk in the
first place. But the point is: we're talking about an adaptive statistical data
- any minor data loss has a small effect which will be compensated for by
further training. But this is a meaningless argument - the timeout approach is a
sound one, and if someone thinks a small timeout is in order then just name a
different value and I would be the last to object.

Comment 30

[Eyal Rozenberg]

I wish someone would review the patch... mscott?...

Comment 31

[Stephen Donner [:stephend] Not actively reading bugmail]

Eyal - fix checked in by timeless on 2005-01-13 12:41.  I'll let you mark it or
not as you see fit.

Comment 32

[Eyal Rozenberg]

I'll make the bet that there are no regressions and mark this fixed.

Comment 33

[Scott MacGregor]

There's a problem with this patch that is preventing the training file from
being updated on exit. By the time the application is shutting down, the file
directory service has already been shut down before the bayesian filter trys to
get the training set file. As a result, we are currently never writing any data
out to traininset.dat on Trunk Thunderbird builds unless you leave the client
running for a long time. 

See Bug #283080

Comment 34

[Tuukka Tolvanen (sp3000)]

>+    rv =
prefBranch->GetIntPref("mailnews.bayesian_spam_filter.flush.diryting_messages_threshold",&mDirtyingMessageWriteThreshold);

pref name typo.

Comment 35

[Scott MacGregor]

that pref name is way too long. I'll shorten it, but I don't see where anyone
actually sets the pref anywhere...

Comment 36

[Worcester12345]

(In reply to comment #35)
> that pref name is way too long. I'll shorten it, but I don't see where anyone
> actually sets the pref anywhere...

Did this ever get shortened? Was that part of the checkin for the fix? Thanks.

Comment 37

[Tuukka Tolvanen (sp3000)]

imo that particular pref should just go, see bug 283493

Comment 38

[Worcester12345]

(In reply to comment #37)
> imo that particular pref should just go, see bug 283493

Got my vote.