better snippets for KerasHub models #1021
Conversation
martin-gorner
requested review from
SBrandeis,
gary149,
Wauplin,
julien-c,
pcuenca and
ngxson
as code owners
| keras_hub_task_json?: { | ||
| class_name: string; | ||
| alt_class_names?: string[]; | ||
| }; | ||
| keras_hub_config_json?: { | ||
| class_name: string; | ||
| }; | ||
| keras_hub_tokenizer_json?: { | ||
| class_name: string; | ||
| }; |
There was a problem hiding this comment.
slight preference for a more concise
| keras_hub_task_json?: { | |
| class_name: string; | |
| alt_class_names?: string[]; | |
| }; | |
| keras_hub_config_json?: { | |
| class_name: string; | |
| }; | |
| keras_hub_tokenizer_json?: { | |
| class_name: string; | |
| }; | |
| keras_hub?: { | |
| // relevant task.json content | |
| }; |
From what I understand, task.json is the future-proof way of getting this info correctly. And getting things from config.json/tokenizer.json is more of a default for previous models up to now. Is my assumption correct or not? If that's the case, then let's focus on parsing only task.json to only promote the "correct" way.
In any case (no matter if the config comes from task.json, config.json or tokenizer.json) I think that having a single field with nested values is better rather than exposing 3 different high-level fields related to keras_hub.
There was a problem hiding this comment.
Taking a look at this recent keras-hub model (https://huggingface.co/evandrarf/health-care-gemma2-kagglex/tree/main), I can see that task.json, config.json, tokenizers.json and preprocessor.json are all set. And the content of task.json is strictly a superset of the other 3. Do you know if other files are kept for backward compatbility?
There was a problem hiding this comment.
I agree with you that standardizing on a single config file would be the best. Let me ask the keras team?
There was a problem hiding this comment.
Matt on the Keras team responded. task.json is not always present and that is by design and not a legacy thing. I recommend we deploy the currently implemented logic while we continue the discussion with Matt and possibly simplify.
There was a problem hiding this comment.
I'd rather wait for a simplification before merging except if it's time-sensitive
There was a problem hiding this comment.
yes and imo we can influence the standardization by supporting the simpler / single-version version that Wauplin mentions
| let class_name = | ||
| // If the model has a task.json config, then the base Task class is known | ||
| model.config?.keras_hub_task_json?.class_name ?? | ||
| // If only a config.json is present, the base class will be a "backbone" | ||
| model.config?.keras_hub_config_json?.class_name; |
There was a problem hiding this comment.
Related to my comment above, if we can get rid of some logic by parsing only task.json, that would be for the best.
Added better "use this model" snippets for KerasHub models with:
The data for this comes from parsing
task.json, and if not available,config.jsonandtokenizer.jsonas well as the pipeline_tag in metadata. The parsing is done in the model repo code.Screenshot attached for Llama3
