Skip to content

Conversation

teddyking
Copy link
Member

@teddyking teddyking commented Aug 13, 2024

About

This PR updates the GenAICfEnvProcessor to incorporate the notion of model capabilities, which will be included in binding credentials from version v0.6.0 of GenAI on Tanzu Platform onwards.

I will leave this PR in draft until v0.6.0 has been officially released. Update: it has now been released.

Details

  • Split the GenAICfEnvProcessor into two - one specific to chat and the other specific to embedding
  • This allows a developer to either make one binding to a model that supports both chat and embeddings, or to make two distinct bindings, one to a chat model and the other to an embeddings model
  • When binding to two different models, each model will have a different api key, which will be set accordingly on the corresponding spring.ai.openai.{chat,embedding}.* properties
  • Both processors additionally set the top-level property spring.ai.openai.api-key="redundant" in order to circumvent an error that is raised when configuring more specific api-keys on the chat and embedding properties
  • If desired, additional GenAICfEnvProcessors may be written to cater to image and audio model options at a later time

* Valid as of GenAI for Tanzu Platform v0.6.0
* Split the GenAICfEnvProcessor into two - one specific to chat and
  the other specific to embedding
* This allows a developer to either make one binding to a model that
  supports both chat and embeddings, or to make two distinct bindings,
  one to a chat model and the other to an embeddings model
* When binding to two different models, each model will have a different
  api key, which will be set accordingly on the corresponding
  spring.ai.openai.{chat,embedding}.* properties
* Both processors additionally set the top-level property
  spring.ai.openai.api-key="redundant" in order to circumvent an error
  that is raised when configuring more specific api-keys on the chat and
  embedding properties
* If desired, additional GenAI CfEnvProcessors may be written to cater to
  image and audio model options at a later time

Signed-off-by: Ed King <[email protected]>
@teddyking teddyking force-pushed the genai-model-capabilities branch from 4ea1af1 to 4fa77fd Compare August 13, 2024 07:31
@teddyking
Copy link
Member Author

cc @svrc

ArrayList<String> modelCapabilities = (ArrayList<String>) service.getCredentials().getMap().get("model_capabilities");
return modelCapabilities.contains("chat");
} else {
return false;

Choose a reason for hiding this comment

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

nitpick: I would just return false without the else block

Choose a reason for hiding this comment

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

nitpick: I would use the interface type List instead of the concrete type ArrayList. My java is also rusty though, is there any specific reason that made you use ArrayList? I think at least you should be able to use List in the left hand side

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @pivotal-marcela-campo, I have removed the redundant "else" blocks. Re: usage of ArrayList, I simply copied from some existing code in the CassandraCfEnvProcessor.

@anthonydahanne
Copy link
Contributor

hello!
@svrc , since you're the first GenAI tile contributor to java-cfenv, could you please have a quick look? validate the idea?
Thanks a lot!

@teddyking
Copy link
Member Author

v0.6.0 of GenAI on Tanzu Platform has been released, so marking this as ready for review.

@teddyking teddyking marked this pull request as ready for review August 14, 2024 07:41
@svrc
Copy link
Contributor

svrc commented Aug 14, 2024

LGTM!

@anthonydahanne anthonydahanne merged commit 8aa8dbb into pivotal-cf:main Aug 14, 2024
1 check passed
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.

4 participants