Skip to content

Conversation

@Neverbolt
Copy link
Collaborator

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to add a unit test with a simpe example capability?

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

do you need it on this MR?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's okay for this merge reqeust (and the existing unit tests are passing so I kinda hoping that it will not break something). You can create a follow-up merge request with a couple of unit tests

returninstructor.from_openai(self.client)

defget_response(self, prompt, *, capabilities=None, **kwargs) ->LLMResult:
defget_response(self, prompt, *, capabilities: Dict[str, Capability]=None, **kwargs) ->LLMResult:
Copy link
Member

Choose a reason for hiding this comment

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

type information for prompt missing

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

the parent class also has no type information for prompt, what should we limit it to? keep in mind the currently commented out code below that is a compatibility layer to allow both string, renderable and full message list support

Copy link
Member

Choose a reason for hiding this comment

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

what would you suggest? I think the code was written by you (:

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

well there was a reason for not adding a type, so that we can vary in the implementations :)

@andreashappe
Copy link
Member

So this:

  • introduces streaming support to capabilities
    • adds a new web use-case using the streaming
    • adds new tables to the logging database which will be used in the future (currently write-only)
    • there is a "new" openai requirement but that was an indirect dependency before already (according to neverbolt)

Copy link
Member

@andreashappeandreashappe left a comment

Choose a reason for hiding this comment

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

commit message could be better.. but it is what it is for now.

@andreashappeandreashappe merged commit 4ea46ea into ipa-lab:mainJun 19, 2024
@NeverboltNeverbolt deleted the openai-streaming branch November 3, 2025 13:08
Sign up for freeto 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.

2 participants

@Neverbolt@andreashappe