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

[DeepVision port] Adding CLIP + pretrained weights #1947

Closed
wants to merge 1 commit into from

Conversation

DavidLandup0
Copy link
Contributor

What does this PR do?

As discussed in #1933 - setting up a draft PR for porting CLIP and associated layers into KCV. Draft PR for now with placeholder main model dump, layers and tests incoming soon. Will tag once ready for review.

Before submitting

  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue? Please add a link
    to it if that's the case. Porting DeepVision into KerasCV #1933
  • Did you write any new necessary tests?
  • If this adds a new model, can you run a few training steps on TPU in Colab to ensure that no XLA incompatible OP are used?

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.

@ianstenbit just tagging so you can follow the progress as it comes in. Otherwise, no need to spend time until it's un-drafted for review :)

Copy link
Contributor

@jbischof jbischof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Left some early comments

)


class CLIPTokenizer(object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this if no where else is an excellent opportunity to import from KerasNLP. We already have a BPE tokenizer and downstream GPT2 tokenizer!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I'll rely on those then :)
Thanks for the heads up!

keras_cv/models/clip/clip_image_encoder.py Show resolved Hide resolved
keras_cv/models/clip/clip_image_encoder.py Show resolved Hide resolved
@DavidLandup0
Copy link
Contributor Author

@jbischof you got in a bit early on the review 😅
This is a dump of the files as they are. I'll remove the underscored, torch imports etc. and open it from draft onto a review in the next few days :)

Good idea on importing portions from KNLP!

@jbischof
Copy link
Contributor

Actually one complexity on borrowing the NLP tokenizer is that it is written in TF and cannot be ported to Keras Core due to tf_text dependency. So maybe you can borrow the Stable Diffusion one instead.

@divyashreepathihalli
Copy link
Collaborator

divyashreepathihalli commented Dec 14, 2023

@DavidLandup0 Thank you for this PR! are you still working on this PR? I will create a feature branch for adding the CLIP model and you can add your changes to that - https://github.com/keras-team/keras-cv/tree/CLIP
This would help other to contribute to this as well.

@divyashreepathihalli
Copy link
Collaborator

@DavidLandup0 I have cherry picked your changes to this branch - https://github.com/keras-team/keras-cv/tree/CLIP

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.

3 participants