-
Notifications
You must be signed in to change notification settings - Fork 15
chore: [OpenAI] Embedding type change to float[]
#353
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
float[]
float[]float[]
…enai-embedding-float
| && Objects.equals(this.index, embeddingsCreate200ResponseDataInner.index) | ||
| && Objects.equals(this._object, embeddingsCreate200ResponseDataInner._object) | ||
| && Objects.equals(this.embedding, embeddingsCreate200ResponseDataInner.embedding); | ||
| && Arrays.equals(this.embedding, embeddingsCreate200ResponseDataInner.embedding); |
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 equality check may continue with Objects.equals, especially if thats what the generator will do
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.
Using Object.equals will be flagged a bug.
...sap/ai/sdk/foundationmodels/openai/generated/model/EmbeddingsCreate200ResponseDataInner.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(index, _object, embedding, cloudSdkCustomFields); | ||
| return Objects.hash(index, _object, Arrays.hashCode(embedding), cloudSdkCustomFields); |
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.
Similar to above, may continue without Arrays.hashCode()
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.
(How) do you plan to realize that in the generator mustache files?
Related SAP/cloud-sdk-java#734
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 am going to take that as the case where there is no flag/indicator to distinguish between an array of primitive and object. So I will revert and stay consistent with all object type handling.
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.
How exactly are you planning to do that? Are you currently working on a (the above) PR or is it a follow-up BLI?
Establishing manual code changes without a clear path to solve it in the generator is creating problems in the future. Same applies e.g. for prompt-registry PoC PR.
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.
We discussed and found a solution in Cloud SDK
rpanackal
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.
The changes required in equals and hashCode for field type float[] needs to be resolved as a minimum in the generator before this PR can be merged.
…enai-embedding-float
…/ai-sdk-java into chore/openai-embedding-float
Context
Prerequisite: SAP/cloud-sdk-java#734
AI/ai-sdk-java-backlog#203.
We are prepping for release. We also later plan to deliver a behaviour change of openai generator that captures array of numbers as
float[]instead ofList<BigDecimal>.The generator will effectively also change the model class in a breaking manner. To avoid breaking changes in the future, we imitate the effect
manually.Feature scope:
Definition of Done
Error handling created / updated & covered by the tests aboveAligned changes with the JavaScript SDKDocumentation updatedRelease notes updated