Skip to content

fix(embeddings): correctly decode base64 data #1448

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

Merged
merged 3 commits into from
Apr 7, 2025

Conversation

RIscRIpt
Copy link
Contributor

@RIscRIpt RIscRIpt commented Apr 5, 2025

  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

According to NodeJS docs Buffer.buffer is not guaranteed to correspond exactly to the original Buffer.

The previous implementation could use buffer garbage while converting bytes to floats.

Additional context & links

https://nodejs.org/api/buffer.html#bufbuffer

Fixes #1446

According to NodeJS docs Buffer.buffer is not guaranteed to correspond
exactly to the original Buffer. [1]

The previous implementation could use buffer garbage while converting
bytes to floats.

[1] https://nodejs.org/api/buffer.html#bufbuffer
@RIscRIpt RIscRIpt requested a review from a team as a code owner April 5, 2025 16:38
@RobertCraigie RobertCraigie changed the title Fix Core.toFloat32Array, Buffer version fix(embeddings): Core.toFloat32Array, Buffer version Apr 7, 2025
@RobertCraigie RobertCraigie force-pushed the fix-to-float32-array branch 2 times, most recently from ff16ce4 to d6152f2 Compare April 7, 2025 10:43
@RobertCraigie
Copy link
Collaborator

RobertCraigie commented Apr 7, 2025

Thanks for the PR! I've pushed a couple commits to fix some formatting and expand our test suite to catch this issue.

I also verified that the first embedding matches the python SDK behaviour.

@RobertCraigie RobertCraigie changed the base branch from master to next April 7, 2025 11:00
@RobertCraigie RobertCraigie changed the title fix(embeddings): Core.toFloat32Array, Buffer version fix(embeddings): correctly decode base64 data Apr 7, 2025
@RobertCraigie RobertCraigie merged commit 58128f7 into openai:next Apr 7, 2025
8 of 10 checks passed
@stainless-app stainless-app bot mentioned this pull request Apr 7, 2025
@RIscRIpt RIscRIpt deleted the fix-to-float32-array branch April 7, 2025 11:31
stainless-app bot pushed a commit that referenced this pull request Apr 7, 2025
* Fix Core.toFloat32Array, Buffer version

According to NodeJS docs Buffer.buffer is not guaranteed to correspond
exactly to the original Buffer. [1]

The previous implementation could use buffer garbage while converting
bytes to floats.

[1] https://nodejs.org/api/buffer.html#bufbuffer

* add tests for embeddings data

* fix formatting

---------

Co-authored-by: Robert Craigie <robert@craigie.dev>
@stainless-app stainless-app bot mentioned this pull request May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking change in v4.91 — embeddings API
2 participants