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

Update PaliGemma2 presets #2004

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

james77777778
Copy link
Collaborator

I think it's a good idea to ensure compatibility with the official naming convention for PaliGemma2.
The official pattern is: pali_gemma2_[3|10|28b]_[variant]_[image_size]

cc @divyashreepathihalli

@github-actions github-actions bot added the Gemma Gemma model specific issues label Dec 5, 2024
@@ -219,3 +222,34 @@
"kaggle_handle": "kaggle://keras/paligemma2/keras/pali_gemma2_pt_28b_896/1",
},
}

# Ensure compatibility with the official naming convention.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep this a lot simpler, just a list of aliases and resolved names. No regexes.

We should also figure out how we don't list the old name in class.presets, that will prevent them from being rendered on keras.io. We don't want to document the old names presumably.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could add something to pali_gemma/__init__.py.

from keras_hub.src.utils.preset_utils import register_presets
from keras_hub.src.utils.preset_utils import register_aliases

# Old names for presets we no longer use but still support.
register_aliases([
    ("pali_gemma2_10b_ft_docci_448", "pali_gemma2_ft_docci_10b_448"),
])

@@ -66,9 +68,10 @@
"path": "pali_gemma2",
"model_card": "https://www.kaggle.com/models/google/paligemma-2",
},
"kaggle_handle": "kaggle://keras/paligemma2/keras/pali_gemma2_3b_ft_docci_448/1",
# TODO: Rename `pali_gemma_2_ft_docci_3b_448` to `pali_gemma2_ft_docci_3b_448`
"kaggle_handle": "kaggle://keras/paligemma2/keras/pali_gemma_2_ft_docci_3b_448/1",
Copy link
Member

Choose a reason for hiding this comment

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

Does kaggle handle redirects with a rename? @divyashreepathihalli

Copy link
Member

Choose a reason for hiding this comment

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

This is probably important to resolve before we merge this, otherwise we need to start thinking about the ordering of things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unable to just rename, we would need to reupload.

@james77777778
Copy link
Collaborator Author

2nd thought: I think it would be best to rename the presets on the Kaggle page to follow the official naming convention.
Using aliases seems redundant to me.

@divyashreepathihalli @mattdangerw WDYT?

@mattdangerw
Copy link
Member

mattdangerw commented Dec 7, 2024

2nd thought: I think it would be best to rename the presets on the Kaggle page to follow the official naming convention.

Renaming sounds good to me. Aliases would only be to ensure backward compat on the old names for keras-hub. Which would keep us from breaking any guides/code using the names we have out today.

I think we should really first figure out whether kaggle will do any redirects after a rename. Otherwise, we will rename on Kaggle and immediately break the current release right? I can follow up with the Kaggle folks monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gemma Gemma model specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants