feat: Add grounding convenience function #378
Conversation
deekshas8
requested review from
jjtang1985,
tomfrenken,
ZhongpinWang and
KavithaSiva
|
While providing ids for When doing a normal chat completion, you also need a deployment_id. Is it possible to do similar things here? |
| /** | ||
| * Represents the configuration for the Document Grounding Service. | ||
| */ | ||
| export interface DocumentGroundingServiceConfig { |
There was a problem hiding this comment.
[q] To understand for myself better. Why did you choose to declare this as an interface instead of a type?
I see the original config that gets generated is a type.
| filters: [ | ||
| { | ||
| id: 'filter-id', | ||
| data_repositories: ['repo1', 'repo2'], | ||
| data_repository_type: 'custom-type' | ||
| } | ||
| ], | ||
| input_params: ['input'], | ||
| output_param: 'output' | ||
| }; |
There was a problem hiding this comment.
[q] I played around a bit with if null values can be set for properties inside filters object. I found that for id and data_repository_type this is possible as all of them resolve to any.
Should we consider also restricting these values to string in our types for useful typehints in autocompletion?
Or do you think this should rather be fixed by the Orchestration team instead?
There was a problem hiding this comment.
This unfortunately comes from the spec. @MatKuhr This seems wrong to me, combining and empty object and enum in schema. Can we suggest an improvement:
SearchSelectOptionEnum:
anyOf:
- type: string
enum:
- ignoreIfKeyAbsent
- type: string
Also here.
GroundingFilterId is any since the spec provides no type.
There was a problem hiding this comment.
I suspect the current state was inspired from this discussion. But in OpenAPI 3.0, the anyOf actually get's completely ignored here as far as I understand, and this is also how the Swagger Editor behaves. It seem there hasn't been much progress on the matter since 😄
Using the above suggestion leads to problems with the Java generator. It can't deal with this at the moment, and it looks similar for the Python.
But I'm also not sure how beneficial this would be. Ultimately, the above is semantically equivalent to just writing type: string. So the only purpose is documentation, which might as well be moved to a description.
➡️ I think we should just remove this from the spec and either declare string with a description, or declare the enum and expect clients to use the appropriate generator option enumUnknownDefaultCase.
There was a problem hiding this comment.
So this actually works in TS ;) The type that gets generated is:
export type SearchSelectOptionEnum = 'ignoreIfKeyAbsent' | string;
The advantage being its extensible and provides autocompletiong. The current 'ignoreIfKeyAbsent' | any; causes loss of type safety.
But if this suggestion doesn't work well with Java/Python, I understand it doesnt make sense then.
Regarding the solution, I also discussed with @tomfrenken. Having string type with a description sounds like a clearer solution. The SDK generator atleast doesn;t support enumUnknownDefaultCase.
For the GroundingFilterId, I was also not sure for what they do with this id and what is it supposed to be (since the spec has no type, it gets resolved to any), and therefore didn;t introduce generating a random guid.
I agree with the suggestion that it should be optional in the spec and if not provided by the user, the service should handle it. We can also do it if they agree not to do it on their end.
There was a problem hiding this comment.
Opened a PR in orchestration repo to fix the GroundingFilterId type to string.
| type: 'document_grounding_service', | ||
| config: { | ||
| filters: [ | ||
| grounding: buildDocumentGroundingConfig( |
There was a problem hiding this comment.
Same updates can be applied to the sample app
Context
SAP/ai-sdk-js-backlog#178.
Please provide a description of what your change does and why it is needed.
Definition of Done