Skip to content
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

CD backports #1318

Merged
merged 4 commits into from
Aug 10, 2023
Merged

CD backports #1318

merged 4 commits into from
Aug 10, 2023

Conversation

chris-ha458
Copy link
Contributor

follow
three commits that each implement
huggingface/safetensors#312

huggingface/safetensors#315

huggingface/safetensors#317

@Narsil do you think they should be separate PRs?
also how should I properly test them locally?
i'd hate to stress the PR build test infrastructure every time

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 9, 2023

The documentation is not available anymore as the PR was closed or merged.

`cargo check` doesnt work on my local configuration from `tokenizers/bindings/node/native`
i don't think it will be a problem but i have difficulty telling
@chris-ha458
Copy link
Contributor Author

for https://github.com/huggingface/safetensors/pull/312
only a single node check fails.
I'll try what the compiler recommends but i'm not sure of it's downstream consequences. But here goes.

@Narsil
Copy link
Collaborator

Narsil commented Aug 9, 2023

Node is 12 and starting failing some time ago. I need to finish my node rewrite (which is done but I was fighting the distribution mecanism and couldn't make it work).

In the meantime we're effectively dropping node, it can't slow the rest down.

Copy link
Collaborator

@Narsil Narsil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chris-ha458
Copy link
Contributor Author

should upload_package: be capitalized differently either here or on safe tensors?

@Narsil
Copy link
Collaborator

Narsil commented Aug 9, 2023

Not really why ? Just what you're doing here is fine, minimal changes in the same way.

@chris-ha458
Copy link
Contributor Author

all tests besides the node pass!

@Narsil
Copy link
Collaborator

Narsil commented Aug 10, 2023

LGTM

@Narsil Narsil merged commit 862046a into huggingface:main Aug 10, 2023
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.

3 participants