-
Notifications
You must be signed in to change notification settings - Fork 33
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
ESM2 Interactive inference #654
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Farhad Ramezanghorbani <farhadr@nvidia.com>
Signed-off-by: Farhad Ramezanghorbani <farhadr@nvidia.com>
from bionemo.core.utils.dtypes import PrecisionTypes, get_autocast_dtype | ||
from bionemo.esm2.data import tokenizer | ||
from bionemo.esm2.model.model import ESM2Config | ||
from bionemo.testing import megatron_parallel_state_utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had this same issue and almost put a thread together for it. The other option (which honestly might be better) is just to move the context manager outside of eval_esm2
) and just ensure that eval_esm2
is called inside one. Thats what I do with assert_model_equivalence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.e., I think we can call into bionemo.testing
in the example notebooks without tach yelling at us, and it makes the creation of the megatron context more explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I moved the context manager outside and into the notebooks, no complain from tach!
nemo_model = Float16Module(nemo_config, nemo_model) | ||
|
||
nemo_output = nemo_model(input_ids, attention_mask) | ||
nemo_output = eval_esm2(ckpt_path, test_proteins, precision=precision) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I like to ensure the tensor inputs to the two models are the same, rather than doing that twice here. Might make sense to decompose your eval_esm2 function into more modular pieces so that's possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured we might run into cases where we iterate on a dataloader, so I separated the setup/teardown to avoid configuring the model every time we call forward.
Now I can also add a method to check the inputs and tokenize them if they are sequences instead of two tensors (input_ids, attention_mask). But I don't think that would look clean. What do you think?
Signed-off-by: Farhad Ramezanghorbani <farhadr@nvidia.com>
Signed-off-by: Farhad Ramezanghorbani <farhadr@nvidia.com>
❌ 7 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Signed-off-by: Farhad Ramezanghorbani <farhadr@nvidia.com>
Signed-off-by: Farhad Ramezanghorbani <farhadr@nvidia.com>
@pstjohn any idea why the golden value tests are not passing? I am getting |
@farhadrgh looks like you're changing the model comparison eval function; I'm guessing we're not getting the same values out. Maybe try running through a debugger to see where the change is happening? |
Description
Type of changes
CI Pipeline Configuration
Configure CI behavior by applying the relevant labels:
Note
By default, the notebooks validation tests are skipped unless explicitly enabled.
Usage
Pre-submit Checklist