add SAELens as a library ⚡ #826
Conversation
Vaibhavs10
requested review from
osanseviero,
SBrandeis,
gary149,
Wauplin,
julien-c and
pcuenca
as code owners
There was a problem hiding this comment.
Looks good to me for now. About code snippet, it is not an obligation to have one. If there is no easy way to describe/compute a code snippet, it's fine not the have one.
Regarding download count, I'd suggest
countDownloads: `path_filename:"cfg" AND path_extension:"json"`,to count downloads on all */cfg.json since they are downloaded one by one for what I understand from download_sae_from_hf.
Last request, could you open a PR to add library_name: saelens in the model card metadata of jbloom/GPT2-Small-SAEs-Reformatted.
| prettyLabel: "SAELens", | ||
| repoName: "SAELens", | ||
| repoUrl: "https://github.com/jbloomAus/SAELens", | ||
| filter: false, |
There was a problem hiding this comment.
QQ: @Wauplin - do you have any recommendations on the best way to track downloads in this scenario: https://huggingface.co/jbloom/GPT2-Small-SAEs-Reformatted/tree/main
There was a problem hiding this comment.
Depends if the general use case is to load only one block or all blocks at once.
- if only one block, better to do as I suggested in add SAELens as a library ⚡ #826 (review)
- if all blocks are generally instanciated at once, then it's better to count downloads only on the first block
path: "blocks.0.hook_resid_pre/cfg.json"
There was a problem hiding this comment.
I'm afraid it is a bit more complicated, on more newer repos (cannot link them atm as they are not public yet) they might just have an npz file. The usage is more layer wise so I think the former suggestion for npz, cfg, json makes more sense.
There was a problem hiding this comment.
Yes, I think the each block provides a different lens so they are used independently. But the structure does not appear to be standardized, so I would merge as is for now (without download counts) and clarify when we can.
There was a problem hiding this comment.
okay, the downloads would need more time to standardise. Given this unblocks a release happening soon. Is it okay if we merge as is for now and revisit in a day when we have more clarity.
| prettyLabel: "SAELens", | ||
| repoName: "SAELens", | ||
| repoUrl: "https://github.com/jbloomAus/SAELens", | ||
| filter: false, |
There was a problem hiding this comment.
Yes, I think the each block provides a different lens so they are used independently. But the structure does not appear to be standardized, so I would merge as is for now (without download counts) and clarify when we can.
|
Opened PRs and some are already merged! |
Do you have examples of merged ones? I can't find any models on https://huggingface.co/models?other=saelens Other than that, feel free to merge. |
|
It has good number of examples now! 👍 |
Follow-up to #826, and (currently) an intermediate PR before `.from_pretrained` support is added for any model on the HF Hub. See jbloomAus/SAELens#234 (comment) for additional discussion.
Couple of quick comments:
The way that snippets work are a bit different in the library as they work via this yaml file and load it directly via a snapshot download: https://github.com/jbloomAus/SAELens/blob/fe987f1d1b06a5f0116e390efc87a69f59e04473/sae_lens/toolkit/pretrained_saes.py#L20
Hence, the snippet doesn't really use the repo name directly, ref: https://jbloomaus.github.io/SAELens/
Checking how to make the download counts work right now.