Conversation
PoC RPTImportant Mentions:
|
foundation-models/sap-rpt/src/main/java/com/sap/ai/sdk/foundationmodels/rpt/RptClient.java
Outdated
Show resolved
Hide resolved
pom.xml
Outdated
| <module>core-services/document-grounding</module> | ||
| <module>core-services/prompt-registry</module> | ||
| <module>foundation-models/openai</module> | ||
| <module>foundation-models/sap-rpt</module> |
There was a problem hiding this comment.
(Major)
The PR title says "PoC", therefore the module shouldn't be released at this level of confidence.
E.g.
<profile>
<id>non-release</id>
<activation>
<property>
<name>!release</name>
</property>
</activation>
<modules>
<module>sample-code/spring-app</module>
+ <module>foundation-models/sap-rpt</module>
</modules>
</profile>
...s/sap-rpt/src/main/java/com/sap/ai/sdk/foundationmodels/rpt/generated/client/DefaultApi.java
Outdated
Show resolved
Hide resolved
Jonas-Isr
left a comment
There was a problem hiding this comment.
Mostly minor issues. Looks pretty good :)
| * | ||
| * <p>A REST API for in-context learning with the SAP-RPT-1 model. | ||
| */ | ||
| public class DefaultApi extends AbstractOpenApiService { |
There was a problem hiding this comment.
(Minor/Question)
This is a very non-descriptive class name (which does not come from the spec itself as far as I can see). Does it make sense to rename that when generating?
There was a problem hiding this comment.
I tried some attempts. Unfortunately, there is no real option to fully rename the generated api classes without modifying the spec file.
Short story: Keep name DefaultApi for now, and wait out for spec to be updated with "tags"/ "x-sap-cloud-sdk-api-name" property (as JS expects).
I found two maven generator plugin options <apiNamePrefix> and <apiNameSuffix>. They can be used to add a prefix or replace the suffix "Api" on all generated api classes.
They may be useful in the future. Currently this leads to names like RptDefaultApi or DefaultRptApi, and never get rid of string "Default". If we accept that, when a "tag" is finally added to the spec, this would be a breaking change for our users.
foundation-models/sap-rpt/src/main/java/com/sap/ai/sdk/foundationmodels/rpt/RptClient.java
Outdated
Show resolved
Hide resolved
foundation-models/sap-rpt/src/main/java/com/sap/ai/sdk/foundationmodels/rpt/RptModel.java
Outdated
Show resolved
Hide resolved
| <useOneOfCreators>true</useOneOfCreators> | ||
| <useFloatArrays>true</useFloatArrays> | ||
| <excludePaths>/predict_parquet</excludePaths> | ||
| <excludeProperties>TargetColumnConfig.top_k</excludeProperties> |
There was a problem hiding this comment.
This excludes a property from being generated in the model class. Currently, top_k is not actually released in SAP RPT service.
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <project.build.outputTimestamp>2025-04-03T13:23:00Z</project.build.outputTimestamp> | ||
| <cloud-sdk.version>5.25.0</cloud-sdk.version> | ||
| <cloud-sdk.version>5.26.0</cloud-sdk.version> |
There was a problem hiding this comment.
(Question)
Shouldn't this come via dependabot update?
There was a problem hiding this comment.
Yes, this happens when we release AI SDK before Cloud SDK.
Context
AI/ai-sdk-java-backlog#350.
PoC for supporting the latest SAP RPT (Tabular AI) service
Feature scope:
foundationmodelsmodulesap-rptintroducedRptClientandRptModelclassspec updateactionDefinition of Done
Error handling created / updated & covered by the tests aboveAligned changes with the JavaScript SDK