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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore Cargo.lock for subfolders #1131

Merged
merged 1 commit into from
Dec 25, 2022

Conversation

hvaara
Copy link
Contributor

@hvaara hvaara commented Dec 23, 2022

No description provided.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Dec 23, 2022

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

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.

Shouldn't we remove tokenizers/Cargo.lock too ?

@hvaara
Copy link
Contributor Author

hvaara commented Dec 24, 2022

I think it already is right? I only see Cargo.toml, not Cargo.lock, in that folder.

@Narsil
Copy link
Collaborator

Narsil commented Dec 25, 2022

My bad !

@Narsil Narsil merged commit 4d520c9 into huggingface:main Dec 25, 2022
Narsil pushed a commit that referenced this pull request Jan 16, 2023
@hvaara hvaara deleted the bindings-sans-cargo-lock branch March 30, 2023 01:53
@kira-bruneau
Copy link

kira-bruneau commented Apr 16, 2023

Hi! I was just wondering why these lock files were removed?

The NixOS build of tokenizers' python bindings uses the lock file to reproducibly fetch the rust dependencies: https://github.com/NixOS/nixpkgs/blob/eafd00c16a2113ca38017f63fd1b348384e422b4/pkgs/development/python-modules/tokenizers/default.nix#L72-L76.

Without it we'd have to manually generate a lock file our self at each version bump, and that may be different than what's actually used in pypi.org.

@Narsil
Copy link
Collaborator

Narsil commented Apr 17, 2023

Because Cargo.lock within the bindings folders are constantly changing, leading to very noisy PRs.

It's the same reason they weren't added in safetensors. As said over there, having reproducing builds is something which is nice.
But the pain of having a constantly changing file which has nothing to do with the code is also real.

In any case, it shouldn't be committed in the library folder (since it's a library) but could be kept in the bindings. Ideally though it should be only present in the release branches which is not the case right now.

@bobvanderlinden
Copy link

Hmm, I ran into the same problem. It's hard to reproduce tokenizers correctly without a Cargo.lock file. I created an issue for this: #1226

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.

None yet

5 participants