use TextEncoder in coca encode_image

[gpucce]

This PR adds the possibility to use a CLS token as a model parameter in TextEncoder to simplify the logic of CoCa

[gpucce]

@rom1504 please don't merge this yet

[gpucce]

@rom1504 this should be ready, essentially it addresses these points that were raised in the old PR and consequently adds possibility for text only encoder to be the Huggingface one, to make the multimodal one also available as HF is more difficult and I would do it in a different PR. As usual reviews, comments and anything else are more than welcome.

would appreciate your review @rwightman if you think anything big need to be done

Made some code review comments, most important points:

• Make CoCa model dual tower (.visual, .text) from the start so builtin and HF text transformer models are the same, no bwd compat to be concerned about
• Revert changes to Attention / CustomResidualAttentionBlock, we should avoid using them with the CoCa model
• Move all models to output dicts instead of tuples, makes it more flexible pass output through optional loss / filters w/o brittle indexing and tuple len checks that are bound to fail with one more addition. Could make the dict output optional at construction time to prevent possible breaks for other downstream users.

and then test test test.

Originally posted by @rwightman in #256 (comment)

[iejMac]

Hey, nice work! Left some very minor comments but I still need to look at the HF stuff in more detail. I'll do that later

[gpucce]

Hey, nice work! Left some very minor comments but I still need to look at the HF stuff in more detail. I'll do that later

Thanks for looking into it! For HF I didn't add a new Pooler but can do it if you think it is better

[iejMac]

For HF stuff from my quick look I have 2 concerns:

1. Do all HF models have resize_token_embeddings?
2. What is the point of embed_cls? We already have a CLS pooler: https://github.com/gpucce/open_clip/blob/0e46a0199d6aaae69dc5c5089a62e8fe3b9d26ab/src/open_clip/hf_model.py#L65

[gpucce]

For HF stuff from my quick look I have 2 concerns:

1. Do all HF models have resize_token_embeddings?

2. What is the point of embed_cls? We already have a CLS pooler: https://github.com/gpucce/open_clip/blob/0e46a0199d6aaae69dc5c5089a62e8fe3b9d26ab/src/open_clip/hf_model.py#L65

1. Should be a method of PretrainedTransformer https://huggingface.co/docs/transformers/main/en/main_classes/model#transformers.PreTrainedModel.resize_token_embeddings

2. The reason would be to have the model CLS token and the one used for contrastive loss to be different. And adding this through the HF tokenizer seems more difficult to make stable.

[iejMac]

@gpucce Re point 2: I think we actually want the CLS tokens to be the same. The idea would be to tune the CLS output embedding so that it's useful contrastively vs. just learning a completely new one. I realize this raises the issue of transformers that don't use CLS pooling but I don't see a problem with diverging from CoCa a bit here i.e. allowing for different pooling methods (like averaging) and using that as the text embedding for contrastive learning. To stay consistent with the paper we can make CLS pooling the pooling method for the default configuration.

[iejMac]

Let me know what you think and also if you're busy. I can get to it myself actually in case you are

[gpucce]

Let me know what you think and also if you're busy. I can get to it myself actually in case you are

Hi, I am getting to it now and I agree with you, I will remove the embed_cls option. Another issue might then be that what is fed to the generative side might not have any <end_of_text> token.

[gpucce]

@iejMac would it be bad to add the output_tokens option to the poolers themselves?

[iejMac]

Why do we want to add it to the poolers? Can't we just use the poolers to get the pooled embedding but also return the tokens on the side?

[gpucce]

Why do we want to add it to the poolers? Can't we just use the poolers to get the pooled embedding but also return the tokens on the side?

I was thinking because with CLS pooling the CLS embedding should not be in the tokens while with Mean/Max maybe yes? Can also pick from the second on for all of them

[iejMac]

Ah do you mean that for CLS pooling the tokens are x[1:] whereas for mean/max it's just x? If so I still think maybe jej ust keep poolers doing only pooling and then we can add some condition where it's like

if type(tokenizer) == ClsPooler:
    toks = model.hidden_state[1:]

or something like that, I forgot the exact syntax

[gpucce]

@iejMac sure I will add it here shortly, I have an idea what the issue is too, later I will try with it a bit and late write here what I find

[gpucce]

@iejMac this should log them separately

[iejMac]

@gpucce we should probably also log the total loss since it's weighted so that might contain important info

[gpucce]

@iejMac my idea about the issue seems like it's wrong, another thing could be that since pad tokens are now ignored in the loss it looks higher. Tomorrow I will try and check for other things, I saw your run seems to be doing ok, if you get any evaluation on a checkpoint please share, and sorry for taking a lot of time today.

[rom1504]

it's quite possible the loss will go down a little later

[iejMac]

@gpucce if you look at the Wandb it looks like this PR consistently achieves much higher loss and slightly lower zero shot imagenet performance than our previous version (in the coca branch). Looking over the code again, do you have any ideas if the is expected and if not, what could be wrong? It looks like the captioning loss is pretty high, too bad we can't compare to previous run.

[gpucce]

@iejMac sorry I didn't have time to work on this sooner and also for wasting some compute:

• higher loss is due to now ignoring pad_tokens and indeed performance is lower but close to the same
• the small decrease in performance is my mistake, I added a projection that doesn't make sense and I am confident a small commit should fix it

Before fixing this and making the model not loadable again, I added a PR to CLIP_benchmark LAION-AI/CLIP_benchmark#63 that works with both the coca branch and this one at this stage, I tested the old checkpoint and I get scores like the following

Bleu_1: 0.225, Bleu_2: 0.086, Bleu_3: 0.037, Bleu_4: 0.017, METEOR: 0.064, ROUGE_L: 0.169, CIDEr: 0.089, SPICE: 0.028

This is not really a good validation but should be enough to check if the model generative part is training, running on an untrained model gets all zeros. If you have time could try and run this with the old checkpoint as I did and with the one from the shorter run that finished a couple days ago if you have one, so we can know that generation is still working?

EDIT: Or of course share the checkpoint and I do it

[iejMac]

@gpucce no worries. I uploaded 2 comparison checkpoints to hf here - https://huggingface.co/iejMac/CoCa_tests/tree/main/checkpoints

epoch_53_new.pt is from latest run
epoch_53_old.pt is from our full run on the coca branch

[gpucce]

@gpucce no worries. I uploaded 2 comparison checkpoints to hf here - https://huggingface.co/iejMac/CoCa_tests/tree/main/checkpoints

epoch_53_new.pt is from latest run epoch_53_old.pt is from our full run on the coca branch

@iejMac here are the scores

	Bleu_1	Bleu_2	Bleu_3	Bleu_4	CIDEr	METEOR	ROUGE_L	SPICE
epoch_53_new	0.1789	0.0363	0.0056	0	0.0342	0.0742	0.1605	0.0281
epoch_53_old	0.2004	0.0696	0.0284	0.0115	0.0704	0.0573	0.1533	0.0233

This ones are a bit worse too but probably for the same reason as the CLIP one is will push the fix in a few hours

[iejMac]

@gpucce let me know when you're happy with the fixes

[gpucce]

@iejMac hopefully it is fine now, if you can try running it so we see what happens it would be very nice

[iejMac]

@gpucce I hope you don't mind me asking but what is this new CoCaEncoderDecoder class? I thought it was only going to be a small change. Why do you think all of this is necessary?

[gpucce]

@iejMac not at all, the new class is just to have only .text instead of both .text and .multimodal_decoder.

More in general the reason the change is larger than I thought is that I would have had to use even more parts of the TextTransformer and VisionTransforner from "outside" with things like if hasattr(self.visual, "smth") ... and it was becoming too much so I moved some of the logic as options inside those,

[iejMac]

@gpucce Ah ok that sounds good. Going to start a run up and send wandb

[rom1504]

main...gpucce:open_clip:encode_in_encoder change compared to main for reference

[rom1504]

new run at https://wandb.ai/iejmac/open-clip/reports/CoCa-ViT-B-32-v2--VmlldzozMjM0Nzg1

merging here

let's continue at #308 to make sure we addressed all the points

[Soonhwan-Kwon]

new run at https://wandb.ai/iejmac/open-clip/reports/CoCa-ViT-B-32-v2--VmlldzozMjM0Nzg1

merging here

let's continue at #308 to make sure we addressed all the points

It seems that epoch_53_new comes from older version, are new run checkpoints from latest model architecture?