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

Error on model.summary() with PaliGemma #1654

Open
bebechien opened this issue May 24, 2024 · 5 comments
Open

Error on model.summary() with PaliGemma #1654

bebechien opened this issue May 24, 2024 · 5 comments
Assignees
Labels
Gemma Gemma model specific issues type:Bug Something isn't working

Comments

@bebechien
Copy link

Describe the bug

model.summary() for PaliGemma will fail when passing show_trainable=True
Works fine without the parameter.
Below is an actual error message.

Preprocessor: "pali_gemma_causal_lm_preprocessor"
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Tokenizer (type)                                   ┃                                             Vocab # ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩
│ pali_gemma_tokenizer (PaliGemmaTokenizer)          │                                             257,152 │
└────────────────────────────────────────────────────┴─────────────────────────────────────────────────────┘
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
[<ipython-input-2-0216a872cf53>](https://localhost:8080/#) in <cell line: 1>()
----> 1 gemma_lm.summary(show_trainable=True)

2 frames
[/usr/local/lib/python3.10/dist-packages/keras/src/utils/summary_utils.py](https://localhost:8080/#) in get_layer_fields(layer, prefix)
    262             fields.append(get_connections(layer))
    263         if show_trainable:
--> 264             if layer.weights:
    265                 fields.append(
    266                     bold_text("Y", color=34)

AttributeError: 'GetItem' object has no attribute 'weights'

To Reproduce
https://colab.research.google.com/drive/1o04v1Xok9892KC-TUOuYh8ToU2wY8RPY?usp=sharing

Expected behavior
Display the model summary with Trainable info.

Additional context
N/A

Would you like to help us fix it?
Sure

@github-actions github-actions bot added the Gemma Gemma model specific issues label May 24, 2024
@SuryanarayanaY SuryanarayanaY added the type:Bug Something isn't working label May 27, 2024
@chunduriv
Copy link
Collaborator

chunduriv commented May 28, 2024

cc: @mattdangerw

@miticollo
Copy link

miticollo commented Jun 13, 2024

This is a semantic issue.

AttributeError is raised here because a keras.ops (like GetItem) doesn't have the weights property.
In Keras, a Layer is an Operation, but vice versa is not true. Anyway, a Functional model can also include raw Keras 3 ops.
Maybe, a dirty workaround could be to add the weights property to Operation class:

@property
def weights(self):
    weights = []
    return weights

IMO this solution is not semantically correct because weights for an operation don't make any sense.

Or an alternative solution could be to wrap the keras.ops into a Lambda layer. I prefer avoiding this layer because of its limitations but maybe in this case it can be used safe.

Finally this issue should be moved in keras repository.

@mattdangerw
Copy link
Member

Thanks @miticollo. Agreed, this should move to the keras repo.

Could we just have models.layers filter down to only return Layer and not Operation? That's what is used by the summary utils, and generally speaking I think having model.layers return something that is not a Layer is just unexpected.

Not sure if anything would break, but we could try it out. @miticollo are you interested in taking it on?

@miticollo
Copy link

Thank u for the answer! At the moment I'm a bit busy with the latest exams at University (one is on Deep Learning). So I can't do it.
Anyway using a filter it could be a good a idea. But currently summary shows also the keras.ops. So is this behaviour still intended?

@mattdangerw
Copy link
Member

mattdangerw commented Jun 17, 2024

@miticollo good point. Perhaps?

Still rubs me the wrong way that model.layers would return something that is not a layer. Users should be able to confidently call any layer method (e.g. layer.count_params()) on everything inside model.layers in a loop. Though potentially we should also have model summary plot ops as well as layers, in which case we should not sure model.layers to loop through everything.

For now I will open a bug in core Keras, assign to myself, and mark open for contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gemma Gemma model specific issues type:Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants