-
Notifications
You must be signed in to change notification settings - Fork 16
feat: [Orchestration] Convenience for grounding metadata params #659
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
Conversation
| } | ||
|
|
||
| @Test | ||
| void testGroundingWithConvenience() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this test since the only grounding unit test I saw was only testing low-level code (not our convenience).
| .filters(filters) | ||
| .metadataParams(metadataParams); | ||
|
|
||
| // metadata_params field is not allowed for data repository type: `help.sap.com` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(minor)
This looks a bit counter intuitive. Instead of setting metadataParams first and then conditionally removing it, we should just conditionally set it.
| .filters(filters) | |
| .metadataParams(metadataParams); | |
| // metadata_params field is not allowed for data repository type: `help.sap.com` | |
| .filters(filters); | |
| // metadata_params field is not allowed for data repository type: `help.sap.com` | |
| val hasHepSapCom = | |
| filters.contains( | |
| DocumentGroundingFilter.create().dataRepositoryType(DataRepositoryType.HELP_SAP_COM)); | |
| if (!hasHepSapCom) { | |
| groundingConfigConfig.setMetadataParams(metadataParams); | |
| } |
Then, you can also set the default value of field metadataParams to null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach unfortunately does not work since if you do not set metadata params explicitly, the generated code will use an empty list which already leads to an error form grounding service when using help.sap.com.
I changed the default value of the field to null but unfortunately do not see how to improve the code snippet you marked (I agree that it doesn't look ideal).
sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/services/OrchestrationService.java
Show resolved
Hide resolved
rpanackal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Question)
Correct me if I am wrong.
So the convenience only for passing the metadata_params in request but not inferring its result out of response, right?
Yes, exactly. And the only way to get the metadata out of the response is by parsing the string of |
Context
AI/ai-sdk-java-backlog#269
This PR introduces convenience for specifying additional metadata parameters for grounding calls through orchestration. It also includes appropriate unit and e2e tests.
Note: The use case for this feature according to the help.sap.com documentation of it is to receive additional information, i.e., the metadata of used chunks/documents, when using grounding via orchestration.
Feature scope:
Definition of Done