Skip to content

[Bug Fix] Create/Update team member api 500 errror #10479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 3, 2025

Conversation

hagan
Copy link
Contributor

@hagan hagan commented May 1, 2025

[Bug fix] /team/member_update API endpoint raises 500 error

Fixes issue raised here: #10477

Note: I have not created tests yet, not sure how to go about testing a database/Prisma issue since regular unit tests don't cut it.. Update: Added simple test for api endpoint and manually tested db below shown

Relevant issues

None that I could find

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have Added testing in the tests/litellm/ directory, Adding at least 1 test is a hard requirement - see details
  • I have added a screenshot of my new test passing locally
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem

Type

🐛 Bug Fix
✅ Test

Changes

Copy link

vercel bot commented May 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2025 10:39pm

@CLAassistant
Copy link

CLAassistant commented May 1, 2025

CLA assistant check
All committers have signed the CLA.

@hagan
Copy link
Contributor Author

hagan commented May 5, 2025

Added unit test, here it is passing locally:
20250505_litellm_budget_endpoint_tests
all tests:
20250505_litellm_all_tests

@hagan
Copy link
Contributor Author

hagan commented May 5, 2025

This should be ready for review. Despite lint check fail, all unit tests appear to be good. (And this does not test the actual issue with Prisma/database which I've provided some curl commands to reproduce in the #10477 issue ticket

@hagan
Copy link
Contributor Author

hagan commented May 6, 2025

Attaching curl tests from bug report #10477... Before this patch, it will give a 500 error
showing_fixed_output_1
showing_fixed_output_2
showing_fixed_output_3

@hagan
Copy link
Contributor Author

hagan commented May 9, 2025

Hi @krrishdholakia, I know you're likely insanely busy. But is there any chance this PR is something that could be reviewed (or get put on a short list of ready to be reviewed by someone). There is no huge hurry, I just don't want to see it get lost in the stream of PR's you're already working through. Thanks!

await prisma_client.db.litellm_teammembership.create(
data={
"team_id": data.team_id,
async with prisma_client.db.tx() as tx:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @hagan can we refactor this into a separate function - and then have an isolated test for just this - it'll prevent bloat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krrishdholakia didn't think you'd be taking a look at this so quickly. Glad you caught this version. I just started to refactoring (somewhat related to a new bug) Found that deleting a budget broke team listing too. I think the delete issue is caused in the schema and _type.py.. Which I imagine you'll also want me to break out as different PR?

And I'm happy to turn this into a function. As for testing, are there examples in your github actions that I can use to do db testing? I ended up breaking out this fix into my team version of litellm: -> https://github.com/cyverse/litellm-docker/blob/main/patch_tests/20250514-patch-test.sh

},
)
elif identified_budget_id is not None:
await prisma_client.db.litellm_budgettable.update(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the major change, correct?

seems like the error is triggered when trying to update budget for an existing member w/ a budget

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this seemed to be the bulk of change to fix the create/update budget

@hagan
Copy link
Contributor Author

hagan commented May 16, 2025

@krrishdholakia I made that change you suggested and added a test to check that_upsert_budget_and_membership does work.

Latest local tests pass:
Screenshot 2025-05-16 135509
Screenshot 2025-05-16 131742
And my checks against a db:
Screenshot 2025-05-16 134757

The lint check is failing, dunno why.. And when I ran the full test suit locally, it now fails. Not sure didn't work either. But appears the github action checked out (minus lint) and almost passed?

@krrishdholakia
Copy link
Contributor

Thanks @hagan monitoring this

@hagan
Copy link
Contributor Author

hagan commented Jun 3, 2025

@krrishdholakia It looks like it passed this time, phew.... Locally when I used make unit-tests it failed. Nothing related to my source changes afaik. And thanks for helping me along with this PR. Let me know if there's any other lingering issues.

@krrishdholakia krrishdholakia merged commit 0f449bf into BerriAI:main Jun 3, 2025
6 checks passed
@krrishdholakia
Copy link
Contributor

LGTM!

Thanks for your help on this

stefan-- pushed a commit to stefan--/litellm that referenced this pull request Jun 12, 2025
* Fixes issue with team_endpoints on member budget update

* refactored location of budget membership fix

* added test for _upsert_budget_membership func
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants