feat: [Tabular] Added Tabular orchestration module - #1028
CharlesDuboisSAP wants to merge 35 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);| import org.junit.jupiter.api.Test; | ||
|
|
||
| @Slf4j | ||
| class TabularTest { |
There was a problem hiding this comment.
(Minor)
Can we rename this to TabularOrchestrationTest? Since also the RptTest class is somewhat about tabular stuff.
There was a problem hiding this comment.
Just the test class or everything?
|
After discussion, I would rather wait until the Predict spec is stabilised and working properly. |
CharlesDuboisSAP
added
dont-merge
and removed
please-review
4 participants
Context
New AI Core service
Definition of Done