feat: [vertexai] adding system instruction support #10775
Conversation
jaycee-li
left a comment
jaycee-li
left a comment
There was a problem hiding this comment.
Thanks Guillaume for implementing this feature and fixing javadoc issues!
| private final GenerationConfig generationConfig; | ||
| private final ImmutableList<SafetySetting> safetySettings; | ||
| private final ImmutableList<Tool> tools; | ||
| private final Content systemInstructions; |
There was a problem hiding this comment.
Can we rename to systemInstruction?
| private GenerationConfig generationConfig = GenerationConfig.getDefaultInstance(); | ||
| private ImmutableList<SafetySetting> safetySettings = ImmutableList.of(); | ||
| private ImmutableList<Tool> tools = ImmutableList.of(); | ||
| private Content systemInstructions = null; |
There was a problem hiding this comment.
According to Java readability reviewers, using null is discouraged. Can we switch to Content.getDefaultInstance() or Optional?
There was a problem hiding this comment.
I'll go with Optional as it's clearer when system instructions have been set or not (otherwise I'd have to examine the Content instance to see if it's a default one or a normal one).
I think it'll be clearer / cleaner that way.
| Content systemInstructions = | ||
| Content.newBuilder() | ||
| .addParts(Part.newBuilder().setText(systemInstructionText).build()) | ||
| .build(); |
There was a problem hiding this comment.
We can use ContentMaker.fromString(systemInstructionText) here
| .thenReturn(mockGenerateContentResponse); | ||
|
|
||
| Content content = | ||
| Content.newBuilder().setRole("user").addParts(Part.newBuilder().setText(TEXT)).build(); |
| GenerationConfig generationConfig, | ||
| ImmutableList<SafetySetting> safetySettings, | ||
| ImmutableList<Tool> tools, | ||
| Optional<Content> systemInstruction, |
There was a problem hiding this comment.
Can we add a setSystemInstruction in the GenerativeModel.Builder class? Also a getter method in the GenerativeModel
| * instructions. | ||
| * @return a new {@link GenerativeModel} instance with the specified tools. | ||
| */ | ||
| public GenerativeModel withSystemInstructions(Content systemInstructions) { |
There was a problem hiding this comment.
Can we replace all SystemInstructions with SystemInstruction?
jaycee-li
changed the title
jaycee-li
changed the title
2 participants
WIP: system instructions support