-
Notifications
You must be signed in to change notification settings - Fork 16
feat: [DevOps] PoC RPT #732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Question)
Shouldn't this come via dependabot update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this happens when we release AI SDK before Cloud SDK.
newtork
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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