Conversation
| * | ||
| * @return The {@link ErrorResponse} object, or {@code null} if not available. | ||
| */ | ||
| @Beta |
There was a problem hiding this comment.
I am not entirely sure why all our custom exception classes are marked beta. I think by now they are pretty stable?
There was a problem hiding this comment.
When the API was introduced we did a lot of major refactoring for error handling.
It wasn't clear for how long that may stick around.
I think it has proven itself reliable (enough) today. We can stabilize.
| * href="https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/http/converter/json/Jackson2ObjectMapperBuilder.html">Jackson2ObjectMapperBuilder</a> | ||
| */ | ||
| @Nonnull | ||
| @Beta |
There was a problem hiding this comment.
There is already a class-level beta annotation
| */ | ||
| @Beta | ||
| @Nonnull | ||
| public OpenAiClient withApiVersion(@Nonnull final String apiVersion) { |
There was a problem hiding this comment.
(Minor)
While I like the API and implementation, I do wonder whether the Beta annotation was used to discourage the usage. If I recall correctly we actually did not want users to use that method - right? We could check back with PO. Maybe there is a productive use-case for that.
There was a problem hiding this comment.
If I recall correctly we actually did not want users to use that method - right?
I don't remember this. But maybe someone else?
There was a problem hiding this comment.
I asked Junjie as well, he does not know of any use-case. So I will keep @Beta here to discourage usage.
...ion/src/main/java/com/sap/ai/sdk/orchestration/spring/OrchestrationSpringEmbeddingModel.java
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/ContentFilter.java
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationFilterException.java
Show resolved
Hide resolved
core/src/main/java/com/sap/ai/sdk/core/common/ClientResponseHandler.java
Show resolved
Hide resolved
| * | ||
| * @return The {@link ErrorResponse} object, or {@code null} if not available. | ||
| */ | ||
| @Beta |
There was a problem hiding this comment.
(Question)
ErrorResponse here is a generated model class. Are we ready to remove beta annotation from api that binds generated classes?
What are we telling our users?
- Since, generated code is unstable, an api with them is inherently unstable, even without beta annotation?
- Or should we deliberately keep the beta in such cases?
There was a problem hiding this comment.
After thinking about this some more I would tend towards 2. What do you and the others think?
There was a problem hiding this comment.
I also agree with 2.
We could take out the class level annotation and instead annotate the method or field appropriately.
There was a problem hiding this comment.
Sounds like a good idea. Would be interested in the opinion of the rest of the team here.
There was a problem hiding this comment.
I'm tending towards(2) too.
But if we do (2) and keep the beta for the reasons described - should we check other APIs as well for consistency? Likely we stabilized public API already in similar situations(?)
There was a problem hiding this comment.
I am a bit torn on this. On one side I think having consistent API is a major point of values the SDK should bring, on the other side it might send weird signals to users if we suddenly mark code beta that was stable before (because they then might assume we will release a replacement soon, or because they assume anything could become beta suddenly). I'll open a Slack discussion on this.
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClientException.java
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClientException.java
Show resolved
Hide resolved
4 participants
Context
As discussed, this PR challenges some of our current
@Betatags.For the ones I did not add a specific comment, I thought the API is fairly established and major changes are unlikely. For all other cases I added some reasoning.
The
@Betatags on the...Apiand...Clientclasses stay since they are more or less just wrappers around generated code.The other
@Betatags I left in where either because they were on pretty new classes/API or because they were on internally used methods and classes.