Skip to content

Vector Embeddings: Add example w/ Cloud SDK for AI #1914

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

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

Conversation

MattSchur
Copy link
Contributor

Adds example code and link to SAP Cloud SDK for AI

@MattSchur MattSchur requested a review from renejeglinsky as a code owner June 10, 2025 14:10
@MattSchur MattSchur requested a review from agoerler June 10, 2025 14:14
Copy link
Contributor

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

otherwise LGTM!

@MattSchur MattSchur requested a review from MatKuhr June 11, 2025 06:53
Copy link
Contributor

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

lgtm

@MattSchur MattSchur enabled auto-merge June 13, 2025 08:39
@MattSchur MattSchur removed the request for review from agoerler June 17, 2025 08:14
@MattSchur
Copy link
Contributor Author

@renejeglinsky could you please review and merge?

@renejeglinsky
Copy link
Contributor

I removed the profiled content as we want to reduce the usage of this toggle and maybe even remove it completely.

@MattSchur MattSchur requested a review from renejeglinsky June 23, 2025 09:25
Comment on lines +278 to +285
:::details Example using SAP Cloud SDK for AI
```Java
var aiClient = OpenAiClient.forModel(OpenAiModel.TEXT_EMBEDDING_3_SMALL);
var response = aiClient.embedding(
new OpenAiEmbeddingRequest(List.of(book.getDescription())));
book.setEmbedding(CdsVector.of(response.getEmbeddingVectors().get(0)));
```
:::
Copy link
Contributor

Choose a reason for hiding this comment

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

@MattSchur Do you know whom we could ask from the Node.js side to provide an example?

).run({
input: 'How to use vector embeddings in CAP?'
});
const questionEmbedding = response.getEmbedding();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MatKuhr AFAIK CAP Node.js expects the vector as String. @johannes-vogel is this still the case?

```
:::
Copy link
Contributor Author

@MattSchur MattSchur Jun 23, 2025

Choose a reason for hiding this comment

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

Suggested change
:::
```js [Node.js]
const response = await new AzureOpenAiEmbeddingClient(
'text-embedding-3-small'
).run({
input: bookDescription
});
const embedding = response.getEmbedding();
/```
:::

@MatKuhr is this correct? In CAP Node.js the vector embedding needs to be a string '[0.3,0.7,0.1,...]' @johannes-vogel please double check

Copy link
Contributor

Choose a reason for hiding this comment

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

response.getEmbedding() returns number[]. Why would a string be expected? That seems unexpected to me.. Especially if its a string representation of an array..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@David-Kunz is the Node runtime still expecting the embedding vector as String?

Copy link
Contributor

Choose a reason for hiding this comment

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

@johannes-vogel @David-Kunz Any input from your side? Otherwise we merge w/o a Node.js version today evening.

@MattSchur MattSchur requested a review from johannes-vogel June 24, 2025 15:59
@MattSchur MattSchur requested a review from David-Kunz June 25, 2025 15:14
@MattSchur
Copy link
Contributor Author

@renejeglinsky can we merge this PR with this release and update the Node.js examples with another PR?

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.

3 participants