-
Notifications
You must be signed in to change notification settings - Fork 31.2k
[WIP] started adding support for evo2 #42210
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @McClain-Thiel, I'm the ex-postdoc who handles a lot of bio models at HF so feel free to ping me if you have any questions, or whenever this is ready for review! |
|
Awesome thanks @Rocketknight1. One quick question: not sure if youve read through the evo repo but they use a custom inference engine + library called vortex. What is the huggingface philosophy on including this as a dependency? Am i going to have to reimplement all the forward stuff in that in base torch? Or can I add it as a dep? |
…n-hugging-face-transformers Align Evo2 rotary handling with HF generation
|
Hi @McClain-Thiel, good question! We generally prefer not to add whole external inference engines. The right approach is probably:
I haven't investigated too deeply, but if the inference lib also does other stuff like searching for multiple sequence alignments, we probably don't want to put all that directly in Transformers, but we could just add info to the model card. It's fine to show examples in the model card using |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, evo2 |
|
@Rocketknight1 So I think I have it mostly working. Gemini 3 did a lot of the heavy lifting but if you look at the tests I actually got the logits from the vortex implementation then use those to verify that the implementation here is close. I can remove that file ofc but thought you might want to take a look. It also passes a simple smoke test (tokenizer is single letter (not constrained to ATGC) but when sampled it seems to give reasonable results (ATCG). Open to more rigorous testing. Any other suggestions? |
|
The main thing I'd suggest now before I do a full review is to convert to modular. Basically, we want each modeling file in Transformers to be standalone, to make it easy for users to hack on them, and so that users don't need to understand ten layers of inheritance and abstraction to understand the code. However, there's a lot of duplicated code this way, and the way we handle that is "modular" files. They're like scaffolds for the actual modeling files - unlike modeling files, imports and inheritance from other models is allowed there. Ideally, when a model is similar to existing models in the codebase, the modular file is very short and mostly just imports/inheritance, but it still can be used to auto-generate a full modeling file (which is the code that actually gets called) Take a look at other modular files in the codebase to get an idea of how it works. In general, "# Copied from" syntax is deprecated now, and we prefer to import in the modular file instead. Once you've made the modular file, running |
What does this PR do?
Fixes # (issue)
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.