feat: Support JSON for consuming orchestration #346
Conversation
| @@ -36,7 +36,11 @@ export class OrchestrationClient { | |||
| prompt?: Prompt, | |||
| requestConfig?: CustomRequestConfig | |||
| ): Promise<OrchestrationResponse> { | |||
| const body = constructCompletionPostRequest(this.config, prompt); | |||
| const body: CompletionPostRequest | Record<string, any> = | |||
There was a problem hiding this comment.
[req] You don't need to cast body to Record<string, any>. If you parse the JSON string, it will essentially be of type CompletionPostRequest (or some subtype), but you can just add the prompt to it right?
There was a problem hiding this comment.
Yes but I was also considering the scenario where perhaps a spec change adds additional parameters which have not been adopted yet in our SDK. Since we are not checking for type safety, it can possibly provide users with a way to run their configurations still. Any thoughts on this?
There was a problem hiding this comment.
provide users with a way to run their configurations
Yes, this is one of the use cases we want to cover with this approach
There was a problem hiding this comment.
Technically, adding Record or not won't have any effect on user, because this body is just a local variable, which will never affect any user.
I would also remove Record<string, any> unless we want to be 100% strictly correct and say the JSON object parsed from the string can have breaking changes.
No strong opinion and I see both points. In the end the type for body does not matter, and return type from constructCompletionFromJson is just for our own concern. The method is internal.
There was a problem hiding this comment.
That is correct. However, I still feel like it reads well to have it since it is explicitly clear to a technical user that the object can contain properties aside from those in CompletionPostRequest. Plus, removing it here would also mean that the return type for constructCompletionFromJson is also of type CompletionPostRequest which, strictly speaking, might not be the case.
But I guess I will defer to your judgement on this @deekshas8
There was a problem hiding this comment.
I am also fine if you remove the type from here completely. Because:
- Removing
Record<string, any>is a misrepresentation of the type ofbody, since we are not enforcing adherence of type returned fromconstructCompletionFromJsontoCompletionPostRequest bodyis not used after this point for any local operations for which type-safety might be needed. Also, the data property of execute function isanywhich makes the union typing here moot.
I would maybe document that body can have additional types for clarity
There was a problem hiding this comment.
Just for documentation, since we now need to be able to dynamically add streaming options, we actually do need some degree of type safety 😬
| // Or alternatively, you can also provide the JSON configuration as a plain string in the code directly. | ||
| // const jsonConfig = 'YOUR_JSON_CONFIG' | ||
|
|
||
| const response = await new OrchestrationClient(jsonConfig).chatCompletion(); |
…dk-js into orchestration-from-json
packages/orchestration/src/orchestration-completion-from-json.test.ts
Outdated
Show resolved
Hide resolved
packages/orchestration/src/orchestration-completion-from-json.test.ts
Outdated
Show resolved
Hide resolved
packages/orchestration/src/orchestration-completion-from-json.test.ts
Outdated
Show resolved
Hide resolved
packages/orchestration/src/orchestration-completion-from-json.test.ts
Outdated
Show resolved
Hide resolved
packages/orchestration/src/orchestration-completion-from-json.test.ts
Outdated
Show resolved
Hide resolved
packages/orchestration/src/orchestration-completion-from-json.test.ts
Outdated
Show resolved
Hide resolved
packages/orchestration/src/orchestration-completion-from-json.test.ts
Outdated
Show resolved
Hide resolved
| @@ -36,7 +36,11 @@ export class OrchestrationClient { | |||
| prompt?: Prompt, | |||
| requestConfig?: CustomRequestConfig | |||
| ): Promise<OrchestrationResponse> { | |||
| const body = constructCompletionPostRequest(this.config, prompt); | |||
| const body: CompletionPostRequest | Record<string, any> = | |||
There was a problem hiding this comment.
I am also fine if you remove the type from here completely. Because:
- Removing
Record<string, any>is a misrepresentation of the type ofbody, since we are not enforcing adherence of type returned fromconstructCompletionFromJsontoCompletionPostRequest bodyis not used after this point for any local operations for which type-safety might be needed. Also, the data property of execute function isanywhich makes the union typing here moot.
I would maybe document that body can have additional types for clarity
jjtang1985
dismissed
deekshas8’s stale review
Shibesh would be off, and said it should be ready to merge.
Context
SAP/ai-sdk-js-backlog#179.
This PR adds a
executeFromJSONmethod that allows users to copy/download the JSON from the Launchpad and use it directly with the SDK. Type safety is not checked as the user takes the responsibility for testing the JSON they use.Definition of Done
[ ] Documentation updated[ ] (Optional) Release notes updated