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

VarBuilder cleanup #627

Merged
merged 4 commits into from
Aug 27, 2023
Merged

VarBuilder cleanup #627

merged 4 commits into from
Aug 27, 2023

Conversation

LaurentMazare
Copy link
Collaborator

This PR cleans up a couple rough edges of the VarBuilder, it tries to preserve the user-facing API so as not to be too much of an intrusive change.

  • This makes the var builder extensible via the Backend trait. All the current implementations are in var_builder.rs but it's easy to move them/add new ones etc.
  • There is a simplified version of the trait for the case when no extra argument is required, this should cover most use cases.
  • The sharding api use the full trait rather than the simplified version, the user facing api has been made more in line with the "normal" version.
  • get_or_init is renamed to get_with_hints so as not to be misleading on potential initialization (this decision is deferred to the backend).

Pending/drawbacks:

  • More documentation to make the concepts easier to grasp.
  • There are lots of explicit lifetime (and a phantom type to help with this). This is only required because of the safetensors backend and seems a bit like an abstraction leak, we should consider using some self-referential hack as it would clean up the var-builder code a lot.

Testing:

  • Ran the multi-process llama example.
  • Ran various examples (llama, falcon, yolo) as well as the tests.

@LaurentMazare LaurentMazare merged commit 4c338b0 into main Aug 27, 2023
11 checks passed
@LaurentMazare LaurentMazare deleted the varbuilder-cleanup branch August 27, 2023 17:03
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.

1 participant