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

Fix the serialization of SequenceSummary block #927

Merged
merged 2 commits into from
Dec 29, 2022
Merged

Conversation

sararb
Copy link
Contributor

@sararb sararb commented Dec 19, 2022

Saving a transformer-based model with SequenceSummary as a post block was throwing an error:

E       NotImplementedError: 
E       Layer SequenceMean has arguments ['self', 'initializer_range']
E       in `__init__` and therefore must override `get_config()`.
E       
E       Example:

Goals ⚽

  • Add a get_config method to the SequenceSummary block

Testing Details 🔍

  • Update the test test_transformer_encoder_with_post to check the serialization of the transformer block defined with a SequenceSummary post block.

@sararb sararb added bug Something isn't working P0 labels Dec 19, 2022
@sararb sararb added this to the Merlin 23.01 milestone Dec 19, 2022
@sararb sararb requested a review from rnyak December 19, 2022 18:40
@sararb sararb self-assigned this Dec 19, 2022
@github-actions
Copy link

Documentation preview

https://nvidia-merlin.github.io/models/review/pr-927

@rnyak
Copy link
Contributor

rnyak commented Dec 19, 2022

fixes the model saving error in #908 due to SequenceSummary as a post block.

@rnyak
Copy link
Contributor

rnyak commented Dec 19, 2022

rerun tests

2 similar comments
@sararb
Copy link
Contributor Author

sararb commented Dec 20, 2022

rerun tests

@sararb
Copy link
Contributor Author

sararb commented Dec 20, 2022

rerun tests

@@ -193,6 +193,10 @@ def __init__(self, summary: str, initializer_range: float = 0.02, **kwargs):
config = SimpleNamespace(summary_type=summary)
super().__init__(config, initializer_range=initializer_range, **kwargs)

def get_config(self):
config = super().get_config()
Copy link
Member

Choose a reason for hiding this comment

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

we may also need to also save the summary argument from the __init__ in this config, and implement from_config to restore the layer to the same state correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I have updated the SequenceSummary to make it more flexible.
The reason I didn't add from_config is that SequenceSummary was used as a base class of the transform blocks SequenceLast, SequenceFirst, SequenceMean, and SequenceClsIndex where the summary parameter was set to a default value for each block.

@rnyak rnyak merged commit 9a29def into main Dec 29, 2022
@rnyak rnyak deleted the save-transformer branch December 29, 2022 14:53
jperez999 pushed a commit that referenced this pull request Dec 29, 2022
* fix serialization of SequenceSummary block, used by Transformer-based models

* Add `from_config` method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants