Fix Slack alerting not working if using a DB #10370
Conversation
This reverts commit af16d86.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
|
Hi @hypermoose i'm able to see slack alerting working with a connected db, What's the error you're getting? |
|
@krrishdholakia there is no error but the periodic 5 second batch send is never initialized so until 512 alerts queue up a send never happens. I can reproduce this easily by deploying a fresh instance of the release on railway setting the required environment variables including STORE_DB_IN_MODEL and SLACK_WEBHOOK_URL and then send a test alert via the UI. It never sends. I made mods to print out what was happening and reploy and found that alerting was being set to true in the init not in update_values so the periodic timer was never started. |
Is there something else I can do to move this PR foward? |
|
Hey @hypermoose yep - just add the corresponding unit tests inside |
I added the first unit test for SlackReporting based on other examples in the integration folder. |
|
@krrishdholakia It looks like the unit tests are real close to the 5 min limit set and get cancelled based on merge size or other factors. Can the 5 minutes be increased? |
Found it and increased. I hope this change finally allow my fix to get merged. |
|
@krrishdholakia can I merge this now? |
@ishaan-jaff Could you help me get this merged? I've been dealing with merge conflicts for days and I think I did everything I was asked. |
Title
Fix Slack alerting not working if using a DB.
Relevant issues
Fixes #10368
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit)[https://docs.litellm.ai/docs/extras/contributing_code]Type
🐛 Bug Fix
Changes
Add starting the periodic tasks in updateValues if alerting_args is updated but adds a flag to make sure dups arent started.