Skip to content
This repository was archived by the owner on Dec 11, 2025. It is now read-only.

adding litellm integration into toolkit ENG-1086#30

Merged
r0ymanesco merged 8 commits into
mainfrom
t7-litellm-toolkit
Sep 30, 2024
Merged

adding litellm integration into toolkit ENG-1086#30
r0ymanesco merged 8 commits into
mainfrom
t7-litellm-toolkit

Conversation

@r0ymanesco
Copy link
Copy Markdown
Contributor

This PR adds our litellm integration PR into the ND python SDK. I implemented this in the most brute forced way by copying the main.py file in litellm directly into our toolkit module. This is because litellm is implemented purely modularly without any classes so I couldn't just inherit a class and modify some methods. The up side is that this is a complete replacement for litellm. A user can simply use it as they are using the regular litellm with the added benefit of supporting notdiamond.

@r0ymanesco r0ymanesco marked this pull request as ready for review September 13, 2024 14:14
@r0ymanesco r0ymanesco changed the title adding litellm integration into toolkit adding litellm integration into toolkit ENG-1086 Sep 13, 2024
Copy link
Copy Markdown
Contributor

@acompa acompa left a comment

Choose a reason for hiding this comment

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

I generally worry about bundling external code with an SDK wholesale in this manner. Let's work together during next sprint to find a lightweight path to introducing litellm support without bringing over their main.py or __init__.py files.

"content": "Hey",
},
]
for model in ND_MODEL_LIST:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I missed this in Devansh's PR, but we should probably use @pytest.mark.parametrize here to separate these tests (and optionally run them in parallel via pytest -n, see details).

@r0ymanesco
Copy link
Copy Markdown
Contributor Author

I generally worry about bundling external code with an SDK wholesale in this manner. Let's work together during next sprint to find a lightweight path to introducing litellm support without bringing over their main.py or __init__.py files.

I think one way to reduce copied code is to only copy completion and acompletion from litellm.main and modify it to support notdiamond. The rest can be "copied" by from litellm.main import *

@acompa
Copy link
Copy Markdown
Contributor

acompa commented Sep 26, 2024

@r0ymanesco Do we still need the enormous main.py file? I do see a star import from main, but would hope to avoid that. Could we instead import * directly from litellm.main, since the library is a dependency?

@r0ymanesco
Copy link
Copy Markdown
Contributor Author

@r0ymanesco Do we still need the enormous main.py file? I do see a star import from main, but would hope to avoid that. Could we instead import * directly from litellm.main, since the library is a dependency?

This is the minimum amount of main.py that we need to keep. It's just 3 methods but those 3 methods are over 2000 lines long

@acompa
Copy link
Copy Markdown
Contributor

acompa commented Sep 30, 2024

@r0ymanesco Do we still need the enormous main.py file? I do see a star import from main, but would hope to avoid that. Could we instead import * directly from litellm.main, since the library is a dependency?

This is the minimum amount of main.py that we need to keep. It's just 3 methods but those 3 methods are over 2000 lines long

Yep, I actually spent part of my flight up north reviewing this further. I agree. Approving to unblock you, and we can chat with the LiteLLM team about this contribution experience.

@r0ymanesco r0ymanesco merged commit 7fee8f0 into main Sep 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants