Added account deactivation on spam detection of profile/bio updates #1484
Conversation
|
@lsabor You mentioned that the 2-10% false positive rate was too high. Should I shoot for a 1-3%? There are some profiles which are hard to distinguish from spam. I'm guessing so few people change their bio that this is probably an ok rate (and people will understand why they were flagged when they are). Only 300 have bios and a comment/prediction of our 210,000 users (granted probably ~50% of those users are spam). For reference, we technically have 210k users, 71k users with bios, 25k users have forecasted. Only ~300 have bios AND have forecasted/commented. Given this, I'm guessing that ~50% of users are spam. Over the lifetime of Metaculus this would have resulted in 6-30 false deactivations who could email us to remedy the situation (probably 50% from people doing something stupid like throwing html into their bio, or making something legit sound fishy). And actually this would be more like 0-10 since they might have changed their bios after the 7 day range Though more iterations couldn't hurt, just curious how much of a difference it will make since they can reach out if there is a problem. |
|
Also looks like I need to apply flake8 formatting. Let me fix that real quick. |
|
@lsabor Who would I invite to request review? I should invite Sylvain for the final QA review because he assigned the ticket to me, but for the preliminary review, will just anyone from the team do? |
users/models.py
Outdated
| send_email_async.send( | ||
| subject="Your Metaculus Account Has Been Deactivated", | ||
| message=textwrap.dedent( | ||
| """Your Metaculus account has been deactivated by an administrator or an automated system. Possible reasons could include | ||
| - Suspicious activity | ||
| - Spam/Ad/Inappropriate content in comments | ||
| - Spam/Ad/Inappropriate content in profile bio | ||
| - Manual review for bot and spam accounts | ||
|
|
||
| If you believe this was done in error, please contact support@metaculus.com and we will reactivate your account.""" | ||
| ), | ||
| from_email=settings.EMAIL_HOST_USER, | ||
| recipient_list=[self.email], | ||
| ) |
There was a problem hiding this comment.
I think we should keep models isolated from business logic like emails sending, interacting with brokers and etc. I'd propose to create a soft_delete_user service in users.services and put this logic there.
There was a problem hiding this comment.
Makes sense! To clarify, you are suggesting just to extract this to another file, but still call it within the soft_delete() function?
There was a problem hiding this comment.
I could go either way on it triggering here.
This means an admin soft deleting users from the admin panel would trigger this. I'm not sure if that's desired.
Then again, soft deleting a user inappropriately should probably give them that info.
users/views.py
Outdated
| async def _ask_gpt_to_check_for_spam( | ||
| bio_plus_websites: str, email: str |
There was a problem hiding this comment.
Could you move all these business-logic functions to utils/services not to overload views.py?
There was a problem hiding this comment.
Yep! I was debating whether it added too much to this file as well.
front_end/src/app/(main)/accounts/profile/components/user_info.tsx
Outdated
Show resolved
Hide resolved
front_end/src/app/(main)/accounts/profile/components/user_info.tsx
Outdated
Show resolved
Hide resolved
users/views.py
Outdated
| ) | ||
| is_spam = "TRUE" in gpt_response | ||
| except Exception as e: | ||
| logger.info(f"AI call failed while checking for spam defaulting to FALSE. Error: {e}") |
There was a problem hiding this comment.
Let's have this be a "warning" for now so we get pinged in slack to catch any rampant issues right away.
After it's stable, we can change it back to "info"
There was a problem hiding this comment.
Ok. I had it 'warning' at first before another comment you mentioned in slack about logging. I'll return it.
There was a problem hiding this comment.
Oh, maybe I've been contradicting myself. Sorry!
My current opinion is that an error from a failed call to an LLM service should be high enough priority we get a slack notification for it. This way, if it constantly fails, we can easily get on the case to fix the code!
If the service succeeds and detects spam, that should only be an info or debug level log because things are operating as they should.
I'm generally much more worried about false positive error as it's super inconvenient if the user is legitmate. |
Just make sure you've already read this since you responded to the wrong comment: #1484 (comment). Though generally I'm down to spend another iteration on spam detection. So to clarify then, what false positive rate are you thinking (since I'm guessing current repercussions are low right now)? To do more general testing I'll randomly sample and verify good profiles from the 300 users with bios and >1 comments/forecasts and randomly sample from the 71k users with bios (though most of those will be spam). Then I can get a more direct false positive rate without eyeballing based on ones it got wrong in my local csv. Regardless, I should probably look deeper into the 71k with bios and no comments/forecasts in case most of these are actually not spam. |
|
Added timeout for gpt, improved the gpt prompt to an estimated ~3% false positive rate (which is only for those submitting in the first 7 days of their account being active). There is an estimated 3-20% false negative rate (didn't evaluate this one as deeply). I moved the spam check to a service file, and moved the users/services.py into users/sservices/common.py. After some research into Dramatiq and a discussion with Hlib, I decided not to make the spam check a dramatic actor to allow for instant feedback to the front end. Some stats: I'll be on travel for the next week, so no promises, but I can probably a few small fixes requested if they are blocking merge. We should probably get approval from Sylvain about the false positive rate. |
There was a problem hiding this comment.
Would be curious to hear whether the skipped tests I left in here as a quick check for the spam prompt should be something I should remove. I could see it as clutter in one way, but could also help with debugging later, and give context for what spam vs good accounts look like.
There was a problem hiding this comment.
It is correct to leave this skipped. I might instead want to move it away from automated testing entirely and instead put it into a command that can be run (e.g. python manage.py spam_testing) or something. Basically, we really shouldn't have our testing suite make api calls to real paid services - espeically if people clone the codebase and then the tests fail because they don't have an openAI api key. Instead, it'd be better to have this as a "run it manually whenever you update the prompting or a new OpenAI model comes out" command. I don't think this is important to put more effort into though since we won't run it often. I'd rather see this MVP get merged with the test skipped than have it delayed and make you spend more hours on it.
This testing module is great though.
| self.assert_response_is_403_with_proper_error_code(response) | ||
| self.assert_user_is_deactivated_and_unchanged(user1, original_bio, new_bio) | ||
| self.assert_notification_email_sent(mock_send_mail, self.user_email) |
There was a problem hiding this comment.
cool, I actually really like this method of testing - Nice!
| logger.info( | ||
| f"AI call failed while checking for spam defaulting to FALSE. Error: {e}" | ||
| ) |
There was a problem hiding this comment.
this should be a logger.warning
| logger.warning( | ||
| f"User {user.username} was soft deleted for spam bio: {bio_plus_website[:100]}... " | ||
| f"The reason was: {reasoning[:100]}... " | ||
| f"It took {duration:.2f} seconds to check. gpt_was_used: {gpt_was_used}" | ||
| ) |
There was a problem hiding this comment.
this should be a logger.info
| is_spam, _ = check_profile_update_for_spam( | ||
| user, cast(UserUpdateProfileSerializer, serializer) | ||
| ) | ||
|
|
||
| if is_spam: | ||
| user.soft_delete() | ||
| user.save() | ||
| send_deactivation_email(user.email) | ||
| return Response( | ||
| data={ | ||
| "message": "This bio seems to be spam. Please contact support@metaculus.com if you believe this was a mistake.", | ||
| "error_code": "SPAM_DETECTED", | ||
| }, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
I think this is the part that Hlib mentioned would be worth running in a Task instead of synchronous.
I think this will still be fast enough, so let's leave it as it is and change later only if necessary. But can you add a comment before line 456 to the effect of "spam detection is done synchronously, if profile updating takes too long, move this to an async task instead"?
On second thought, with what you've said I think this failure rate is totally acceptable. Let's not change anything until someone complains.
No need. Sylvain should be informed to do QA on the results live in production (or dev if it's deployed there first). But Sylvain won't do any code review, so pinging him here isn't worth it. Instead, move the ticket to QA and ping Sylvain it's ready for him to close if he's happy with the initial results. |
…(the test data of 322 users with at least 1 comment/prediction and a bio isn't all good bios, so I'm guessing a 2-5% false positives)
…ther refactors as well
…positive rate (from gpt alone w/o date filter)
decf2d1 to
1990a83
Compare
Issue: #1386
New spam detection flow:
Identification rates: