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

Add gen reg tests #689

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Conversation

gpucce
Copy link
Contributor

@gpucce gpucce commented Oct 22, 2023

This adds regression tests for generative models.

@rwightman this should be almost done, however there seems to be a regression error for coca that I had not noticed

@gpucce gpucce marked this pull request as draft October 22, 2023 12:23
@rwightman
Copy link
Collaborator

@gpucce do you have any idea what might be causing it? what's the symptom and by how much is it 'off'? there are numerical changes across versions of pytorch, etc so some difference is expected

@gpucce
Copy link
Contributor Author

gpucce commented Oct 22, 2023

@gpucce do you have any idea what might be causing it? what's the symptom and by how much is it 'off'? there are numerical changes across versions of pytorch, etc so some difference is expected

@rwightman I have been keeping testing and I believe it might be just be me still doing smth wrong with github ci and not a real issue. When I run small tests locally it seems that everything is perfectly equal. I will keep trying to see if I manage to get the whole thing working.

@rwightman
Copy link
Collaborator

@gpucce have you run same random inputs through the different towers, save results to verify closeness within some float eps on same env but with current main and previous release?

ie something along these lines

torch.manual_seed(0)
img = torch.randn(16, 3, 224, 224)
text = torch.randint(0, vocab_size, (16, 77))
outputs = model(img, text)
torch.save(outputs)
...

@gpucce
Copy link
Contributor Author

gpucce commented Oct 22, 2023

@gpucce have you run same random inputs through the different towers, save results to verify closeness within some float eps on same env but with current main and previous release?

ie something along these lines


torch.manual_seed(0)

img = torch.randn(16, 3, 224, 224)

text = torch.randint(0, vocab_size, (16, 77))

outputs = model(img, text)

torch.save(outputs)

...

@rwightman yeah, everything I do by hand seems to be fine. It's with making proper regression tests that I get errors, will check again later or tomorrow, now I must be doing smth dumb but can't find out what

@rwightman
Copy link
Collaborator

rwightman commented Oct 22, 2023 via email

@rwightman
Copy link
Collaborator

FWIW using your cat.jpg, I get '<start_of_text>a cat sitting on its hind legs looking up . <end_of_text>' for both PT 2.1 w/ transformer 4.34 and latest main branch AND same on PT 1.13, transformers 4.24, open_clip 2.16.2.

@gpucce
Copy link
Contributor Author

gpucce commented Oct 22, 2023

@rwightman I think I found it, could it be the new open_clip.tokenize generates sequences with length 76 in some cases?

@gpucce
Copy link
Contributor Author

gpucce commented Oct 22, 2023

@rwightman I think I found it, could it be the new open_clip.tokenize generates sequences with length 76 in some cases?

specifically open_clip.get_tokenizer("coca_ViT-B-32")("some text") has shape [1, 76] in the current version but it had shape [1, 77] in v2.22.0

@rwightman
Copy link
Collaborator

@gpucce I'd avoid using the singleton tokenizer by calling the open_clip.tokenize(), and us factory to get one for your model.
But yeah, the coca configs say context length is 76 so the get_tokenizer will return a tokenizer that outputs 76. context_len is used by tokenizer now as we have multiple models with different context lengths now.

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.

2 participants