Port models to core #1119
Port models to core #1119
Conversation
ianstenbit
left a comment
ianstenbit
left a comment
There was a problem hiding this comment.
Some minor questions for you, but looks good!
Way to go Matt 🎊 🥳 ❗
| self.backbone.get_layer("token_embedding").embeddings, | ||
| transpose_b=True, | ||
| ) | ||
| logits = self.get_layer("reverse_embedding")(x) |
There was a problem hiding this comment.
Just curious, as I see this is done throughout -- what's the purpose of using self.get_layer instead of storing the layer as a member on the object directly e.g. self.reverse_embedding?
There was a problem hiding this comment.
Because we are functional, they actually aren't self.layer or self.backbone.layer accessors.
| ) | ||
|
|
||
| @pytest.mark.large # Saving is slow, so mark these large. | ||
| @pytest.mark.tf_only |
There was a problem hiding this comment.
Is it because of the tokenizer that this is TF only? If so, can we potentially make the test work on other backends?
There was a problem hiding this comment.
(Same throughout the PR)
There was a problem hiding this comment.
This is because a string model inputs are only supported in tf. So a model that includes tokenization inside call really only makes sense in tf.
I am also open to just deleting these tests. I think saving for backbone models and tasks are important, for tokenizers saving directly like this less so.
There was a problem hiding this comment.
We could potentially rewrite the test with preprocessor=False to make it generic
There was a problem hiding this comment.
Oh this is actually a test for just the tokenizer. So preprocessor=False would not apply here.
| ) | ||
|
|
||
| @pytest.mark.large # Saving is slow, so mark these large. | ||
| @pytest.mark.tf_only |
There was a problem hiding this comment.
We could potentially rewrite the test with preprocessor=False to make it generic
We want to avoid a bug in the case that looks like model.generate() mode.fit() model.generate() In this case we need to be careful to not pull in the cached variable state at generation compile time.
* Port models to core * Proper seed generation for jax * Don't test metrics yet (for a separate PR) * Add all model variables to the jax state mapping We want to avoid a bug in the case that looks like model.generate() mode.fit() model.generate() In this case we need to be careful to not pull in the cached variable state at generation compile time. * Address Ian's comments * Add TODO's for revers embedding * Run pytest on the entirety of keras-nlp * Misc cleanups * Mark docstring tests tf only * Last failing doctest
4 participants
🚧 This is an experimental feature branch, more details soon. 🚧
I'm temporarily basing this on
preprocessingbranch so we can start review.