Skip to content

Deprecate Multi Modal LLMs #19115

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

AstraBert
Copy link
Member

@AstraBert AstraBert commented Jun 17, 2025

Description

Deprecate Multi Modal LLMs in favor of multi modal support of their sibling LLM classes.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 17, 2025
@AstraBert AstraBert marked this pull request as draft June 17, 2025 19:27
@AstraBert AstraBert marked this pull request as ready for review June 18, 2025 16:19
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 18, 2025
@AstraBert AstraBert marked this pull request as draft June 19, 2025 11:23
@AstraBert AstraBert marked this pull request as ready for review July 2, 2025 14:53
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jul 2, 2025
@@ -116,20 +116,23 @@ def __call__(
async def acall(
self,
llm_kwargs: Optional[Dict[str, Any]] = None,
image_documents: Optional[List[ImageNode]] = None,
image_documents: Optional[List[ImageBlock]] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably continue to handle passing in ImageNode here and just translate it to ImageBlock

@@ -130,19 +130,22 @@ def evaluate(
context_str=context_str, query_str=evaluation_query_str
)

image_nodes: List[Any] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

slightly misleading name (and if we can avoid typing things as Any, that'd be good too)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(This pops up in a few places)

for image_node in image_nodes
if isinstance(image_node.node, ImageNode)
],
image_documents: List[Any] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets make the variable name and typing here better

@@ -192,154 +184,165 @@ def _get_response_token_counts(self, raw_response: Any) -> dict:
}

def _complete(
Copy link
Collaborator

Choose a reason for hiding this comment

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

for complete and stream_complete, I think we can just translate into message dicts, call chat, and convert the chat response to completion response?

There are a few utils for doing this in core that other LLMs use

Copy link
Collaborator

@logan-markewich logan-markewich left a comment

Choose a reason for hiding this comment

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

This seems pretty good! And holy a huge amount of work, thanks for tackling this

I think there is just some nits on core and maybe anthropic, but otherwise, looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants