Skip to content

[safetensors] better RE_SAFETENSORS_SHARD_FILE #605

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

Closed
wants to merge 1 commit into from

Conversation

mishig25
Copy link
Collaborator

@mishig25 mishig25 commented Apr 4, 2024

Follow up to #593

Two updates:

  1. Extract shard numbers like in [gguf] RE_GGUF_SHARD_FILE #601
  2. Explicitly capture - or _ character present at the beginning of shard substring

@mishig25 mishig25 requested a review from coyotte508 as a code owner April 4, 2024 15:21
@mishig25 mishig25 requested a review from julien-c April 4, 2024 15:23
@@ -14,7 +14,7 @@ export const SAFETENSORS_INDEX_FILE = "model.safetensors.index.json";
/// but in some situations safetensors weights have different filenames.
export const RE_SAFETENSORS_FILE = /\.safetensors$/;
export const RE_SAFETENSORS_INDEX_FILE = /\.safetensors\.index\.json$/;
export const RE_SAFETENSORS_SHARD_FILE = /\d{5}-of-\d{5}\.safetensors$/;
export const RE_SAFETENSORS_SHARD_FILE = /[-_]?(\d{5})-of-(\d{5})\.safetensors$/;
Copy link
Member

Choose a reason for hiding this comment

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

I don't get [-_]?, since it's optional it doesn't do anything?

Maybe remove the ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't get [-_]?, since it's optional it doesn't do anything?

Screenshot 2024-04-05 at 10 34 38 src here

on example above, I'd like to think that - is part of -xxxxx-of-xxxxx.safetensors rather than model-. The reason of this decision is that, if in the future, we wanna show common name of those tensors files it should be model rather than model- & with this regex we can achieve that by str.slice(0, -str.match(RE_SAFETENSORS_SHARD_FILE).length)

Maybe remove the ?

- or _ are optional. So we need to have ?. For example there can be model00002-of-00072.safetensors

Copy link
Member

Choose a reason for hiding this comment

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

hm ok but that's a tenuous dependency between hub.js and the hub. It should be made an explicit function extractShardTensorsData(filename) that returns everything (the root, the numbers, ...) in a typed JSON.

Otherwise assumptions made in one codebase about another codebase can easily be broken, especially if there's no comment / warning that some things are there for a particular reason

Copy link
Member

Choose a reason for hiding this comment

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

i don't think [_-] can really be optional, i've always seen either of them

Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

i think this one can be closed in favor of #622

@mishig25
Copy link
Collaborator Author

Superceded by #622

@mishig25 mishig25 closed this Apr 12, 2024
@mishig25 mishig25 deleted the RE_SAFETENSORS_SHARD_FILE_v2 branch April 12, 2024 08:28
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