I'm not 100% sure we need a COVID detection match/block and I see no statistics supporting it that I can tell. (I expect PRs to include some level of research/statistics to see if it's a good fit)
looking for someone else to headscratch on this too
@ThomasWard I've looked at it briefly, & wondered similar things. I wasn't aware there was a problem which we weren't currently detecting. It looks like an involved detection. I'm hoping we'll see some text in the PR to explain what need it's solving. For example, examples of posts it's intended to detect. It looks like a lot of work went into it, so I'm not wanting to reject it out of hand, but more explanation would help. I also wanted to take a more detailed look at it, the logic, and regexes.
@ThomasWard If I don't have a comment on it today, please remind me. At a minimum, I have some system issues that I need to deal with first, but I'm likely to get distracted onto other things. However, it is my intent to get to it fairly soon; at least today.
@ThomasWard I'm having that same error, combined with E:NotEnoughSleep. :)
@Makyen I self-requested a review and made comments - also marked as "changes requested" without pointing at a specific code line stating we need more info such as what it would detect (with examples), why we need it, how it improves things, etc.
because adding this at the code level is Not Trivial to revert
and may or may not require someone to punt the code around
@Daniil nah i think the issue with Research in this case is PR #4158 doesn't have any nor does it have justification for the magic numbers its using NOR does it seem to fix anything specific that I can tell
@ThomasWard The idea is because we have little data for now (all deleted); and we will have a bunch of them in MS some time later; I believe it has been several times one have to manually report some posts.
The issue is we have no data at all currently; if we get a bunch of fps then that's good; as we can then apply statistical methods to get better constants; if we don't, that's even better
@Daniil Sure, if you'd like to do some testing. However, PR4158 really isn't in a state, yet, where we should be testing. We should get a bit more information from @user12986714 prior to testing (discussion ongoing), but it will need to be tested prior to being merged. If it hasn't already happened, then it will probably be good to have it run for a while as the only detection in an SD instance to get a feel for what it's reporting.
If you're interested, there are other PRs that could be tested. An easy one would be PR 3971, which should be run on an instance for a day, or so, to see how much space the text file actually takes up.
@user12986714 I can see the same FP wave with random 'magic numbers'. However, even if we have a huge wave of FPs, we then will have accurate data from which we can determine Magic Numbers. This being said, I would strongly suggest we do Watches instead of adding a new detection pattern - at least at first
@Daniil Instead of bodyfetcherQueueTimings.p, it creates a new text file, bodyfetcherQueueTimings.txt. It would be helpful to know how large the file gets in a typical day.
> I'd appreciate more comments that explain what your intent is. In reviewing, I shouldn't need to dig through the code cold, having to figure out how what you are implementing is intended to accomplish your overall goal; nor should it be necessary to dig through each section of code or function in order to get a general overview of what is supposed to be happening. It doesn't take a lot of comments, but there should be some guide the person reading the code as to what is intended...
> ... to be happening and in general how that's being accomplished
FURTHER, you need to explain what problem your code changes in a PR are solving, how they're beneficial, and why they're needed.
especially when you're making sweeping changes or large scale changes (like your git refactor - I fail to see why you need to refactor it as is and don't see your justifications)
@ThomasWard I have planned to take a detailed look at that one, and, frankly, all of them again. All of them need at least one more detailed pass through the code. Most still need to at least be tested on an SD instance prior to being merged.
@ThomasWard True; and I think it might be better to use a simple bayes filter/winnow to classify specifically such posts than manually figuring out numbers, once we have data.
@ThomasWard At least on most of them, if it appears they can be improved to the point they might be merged, I'd prefer requesting changes, rather than closing. Closing makes them significantly harder to find/track in the plethora of watch/blacklist PRs, and has some undesired side-effects when updates/improvements are made to the PR (e.g. if a push --force is done to a closed PR, then the PR can never be reopened).
@Makyen true, but most of the things I've seen have zero detections (manual or otherwise) and I usually go through and do a Needs Changes to major things
but a watchlist request that has been open for > 7 days and has no activity and zero recent detections that match it are more simple to handle. (One post 3 years ago is not a good indicator)
(esp. if it's a Smokey issued change/request)
and of course
if someone says "PR no longer working due to unknown reasons" that means, in my mind, they've given up on it
@ThomasWard I just wrote a very simple bayes and winnow classifier, but... we need more data to actually train those model (no ANNs/SVMs because they are way to expensive on cpu/mem)
@user12986714 You don't have blacklist manager privileges, but I've created PR#4163 for you.
PR#4163 ("user12986714: Watch covid(?:-?19)?|corona\W*+virus|pendamic|epidemic|hospitalized?|respirat(?:ory|ion)|oxygen\W*+saturation|fever|septics?\W*+shock|(?:organ|lung)\W*failures?(?#Data collection for covid-19 trolling detection)") opened by SmokeDetector
@user12986714 Thank you for updating this. I'd be a lot more comfortable with this if we were to run an SEDE query to see how many existing users match that criteria. However, I'm not against adding this and collecting some data. I/we have added similar watches for relatively broad username criteria. The important thing is to stay on top of it and make sure we're fpuing users and/or that we remove it, if it's getting a lot of FP.
This is relevant to everyone who uses MS, so just wanted to share that I made a PR to move some stuff around in the navbar to make it less long. Please take a look if you care about the MS navbar.
@ThomasWard Just FYI:, while the deploy button worked for you the last time you tried it, it did not work for thesecretmaster when it was tried a day or two ago.
@Makyen right, that suggests Travis and things're fubar
and the auth keys too
so
@Undo unless you can delegate access to the Travis "Force" button to people, in which case give me and others access. Not sure how that works on your end :p
Oh, not too urgent. Worst case somebody goes ahead and does it, then we gotta patch things back together from a dump. But yeah ideally hold off on deploy until I merge 753, but if you need to deploy, go for it.
@thesecretmaster no, not like the security issues we had with the SQL dumps which required me to instantly push that stuff out for security / privacy reasons
but the advantage of direct system access there is I can circumvent things when i really have to deploy things
if it were a critical or high level security issue I'd be yelling for Art to get online and manually deploy xD
