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.

Sign up for GitHub

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

Jump to bottom

Use cargo.lock to make the project reproducible #229

Closed
MatthewCroughan opened this issue Apr 11, 2023 · 5 comments · Fixed by #233
Closed

Use cargo.lock to make the project reproducible #229

MatthewCroughan opened this issue Apr 11, 2023 · 5 comments · Fixed by #233

Comments

@MatthewCroughan
Copy link

Building the project at two different times of day may produce very different results, because the project is not storing a lock file, and it is in the .gitignore. It would be helpful for projects like my own which attempt to make incredibly complex generative AI software more reproducible (https://nixified.ai) if you did this.
@Narsil
Copy link
Collaborator

Narsil commented Apr 11, 2023

It don't think we should: rust-lang/cargo#315

Librairies need to not have Cargo.lock afaik so that ultimate binaries can get better resolution.
Is that what you're referring to ?

@max-privatevoid
Copy link

As safetensors is a Python library rather than a Rust library, it should absolutely lock its Rust dependencies. Consumers of this library will not have a Cargo.lock in their projects, as they are not using cargo to install safetensors.

@Narsil
Copy link
Collaborator

Narsil commented Apr 12, 2023

It is both.

We could freeze the Cargo.lock for the python package. Would that be enough ?

@Narsil Narsil mentioned this issue Apr 12, 2023
Merged
@max-privatevoid
Copy link

That will be enough to serve our purpose. Will this lockfile be kept up-to-date as the project evolves?

@Narsil
Copy link
Collaborator

Narsil commented Apr 12, 2023

Yes.

If it becomes annoying (Because of constant modifications to it) we could keep it only for the released branches, which I think would be enough.

9999years added a commit to 9999years/safetensors that referenced this issue Apr 18, 2024
@9999years
Add Cargo.lock (again)
649fdd8 
`Cargo.lock` files were added in huggingface#229 and then removed about a month
later in huggingface#253 (with the justification that "cargo.lock messes up
benchmarks").

Shortly after, the Rust project guidance was updated to encourage
committing lockfiles:

https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html
https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control
rust-lang/cargo#8728

Let's add the lockfiles back to make builds reproducible and
deterministic, especially for consumers of Python bindings.
@9999years 9999years mentioned this issue Apr 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Committing Cargo.lock. huggingface/safetensors
3 participants
@Narsil @MatthewCroughan @max-privatevoid