feat: [TabularOrchestration] Added Tabular orchestration module - #1028
CharlesDuboisSAP wants to merge 42 commits into
Conversation
Jonas-Isr
left a comment
Jonas-Isr
left a comment
There was a problem hiding this comment.
New convenience API looks good! Besides that had only smaller comments.
Once the spec seems stable we can release this in my opinion :)
# Conflicts: # docs/release_notes.md
@Jonas-Isr I think we could already merge it as beta. |
| : getService().getInferenceDestination(resourceGroup).forScenario(PREDICT_SCENARIO_ID); | ||
| val destination = | ||
| DefaultHttpDestination.fromDestination(inferenceDestination).headers(customHeaders).build(); | ||
| return new PredictApi(ApiClient.create(destination)); |
There was a problem hiding this comment.
(Question)
This means, the resource group is set once for the PredictApi object. I assume this is because the two specs are different, right?
That's a bit annoying because of the inconsistency in usage but I like the way the public API looks, given this inconsistency is nothing we can change.
There was a problem hiding this comment.
No, it's because if I let the spec define the resource group, then the code generates like so:
new TabularClient().predict().predict(resourceGroup, request);The first predict gets the deployment on the default resource group, then the request with custom resource group will fail because it uses the default deployment.
Now if you set the resource group for fetching the deployment:
new TabularClient().predict(resourceGroup).predict(resourceGroup, request);Then the resource group propagates to the client and it's set twice, again the request fails because AI Core doesn't like it.
That's why I chose setting it once for the client and let it propagate to the request which is inconsistant:
new TabularClient().predict(resourceGroup).predict(request);
|
After discussion, I would rather wait until the Predict spec is stabilised and working properly. |
CharlesDuboisSAP
added
dont-merge
and removed
please-review
ZhongpinWang
left a comment
ZhongpinWang
left a comment
There was a problem hiding this comment.
please consider separate context-registry and tabular-ai-orchestration into two different modules. They are two independent services.
Also rename the spec names, and folders. Context registry is not tabular orchestration. Tabular AI orchestration has one endpoint called /predict but the service is not called tabular predict.
CharlesDuboisSAP
changed the title
# Conflicts: # docs/release_notes.md
I created 2 modules: tabular-ai-orchestration and context-registry.
We should align on whether we prefer the name tabular-ai-orchestration or not.
| @@ -0,0 +1,773 @@ | |||
| openapi: 3.0.3 | |||
| info: | |||
| title: Tabular AI Orchestration | |||
There was a problem hiding this comment.
The specs should be updated.
|
|
||
| // @Test | ||
| // void testCustomHeaders() { | ||
| // WireMock.stubFor( |
There was a problem hiding this comment.
This test should be created using e2e test payloads
5 participants
Context
New AI Core service
Definition of Done