Skip to content
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

early prototype #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

early prototype #1

wants to merge 1 commit into from

Conversation

michalkozminski
Copy link

No description provided.


@Getter
@SuperBuilder
public class AbstractSharedBedrockChatModel {

Choose a reason for hiding this comment

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

[Sugesstion]

Instead of the shared model and then extending the other classes with complex inheritance. I think we could use composition (at least in the scope of our changes).

I see such an implementation with a util class here: https://github.com/beekpr/langchain4j/blob/main/langchain4j-ollama/src/main/java/dev/langchain4j/model/ollama/OllamaMessagesUtils.java

Copy link
Author

Choose a reason for hiding this comment

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

@anandbhaskaran that was my first thought, but then I didn't see this pattern in other libraries, I'll check maybe how other integrations are written.

Copy link

@anandbhaskaran anandbhaskaran left a comment

Choose a reason for hiding this comment

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

Overall this PR will give us a working implementation. 🥳 Great job @michalkozminski

If this implementation is accepted or langchain implements the streaming model let's aim to sunset this fork (just to make sure that we are not stuck in an older version)

@michalkozminski michalkozminski force-pushed the tmp-streaming branch 3 times, most recently from 6d20783 to 5d2bc75 Compare February 27, 2024 09:00
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.

2 participants