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.

Sign up for GitHub

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

Jump to bottom

feat: Orchestration image support #294

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

feat: Orchestration image support #294

wants to merge 60 commits into from

Conversation

Jonas-Isr
Copy link
Member

@Jonas-Isr Jonas-Isr commented Jan 23, 2025 •
edited
Loading

Context

AI/ai-sdk-java-backlog#152.

We want to support using images and prompts with multiple text messages as input for Orchestration.

Feature scope:

  • Added possibility of using image and multiple text messages in prompts
  • Added Convenience layer

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Aligned changes with the JavaScript SDK
  • Documentation updated
  • Release notes updated

@Jonas-Isr

This comment was marked as outdated.

Sign in to view
@Jonas-Isr Jonas-Isr self-assigned this Jan 27, 2025
newtork
newtork reviewed Jan 28, 2025
View reviewed changes
orchestration/src/main/java/com/sap/ai/sdk/orchestration/Message.java Outdated
Comment on lines 76 to 77
default ChatMessagesInner createChatMessage() {
if (this.getContent() instanceof MessageContentSingle) {
return ChatMessage.create().role(role()).content(content());
} else if (this.getContent() instanceof MessageContentMulti mCMulti) {
if (this.content() instanceof MessageContentSingle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Minor/Opinion)

I think it's a mistake to have this method on public API.
Maybe we could address this in another PR.

@Jonas-Isr

This comment was marked as outdated.

Sign in to view
newtork
newtork reviewed Jan 28, 2025
View reviewed changes
Copy link
Contributor

@newtork newtork left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I want to explain my assumptions for convenience API behavior / contract.

  • For request:

    • the user will only use public constructors and factory methods for easy instantiation.
    • low API hierarchy requirement; we'll try to avoid users to define helper classes.
    • strict value checking on design time; only allow what's aligned in spec.

  • For responses:

    • developer uses public accessors.
    • while casting may be necessary, let's try to minimalize the effort.
    • hierarchy can be high as long as it's reasonable.
    • loose value checking; whatever is sent back from server can be read with our API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newtork newtork newtork left review comments

@CharlesDuboisSAP CharlesDuboisSAP CharlesDuboisSAP left review comments

@MatKuhr MatKuhr MatKuhr left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Jonas-Isr Jonas-Isr

Labels
please-review Request to review a pull-request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Jonas-Isr @newtork @MatKuhr @CharlesDuboisSAP @bot-sdk-js