Skip to content

Conversation

jbischof
Copy link
Contributor

@jbischof jbischof commented Aug 9, 2022

Following the general design pattern of keras-cv.

  • Make Bert first model in models/ folder
  • Make Bert Model class using functional API
  • Make BertBase configured in code
  • Make BertClassifier API

Test plan: unit tests + ran examples/bert/* by hand.

Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Thanks! Left some comments. Just focusing on the high level for now.

@jbischof jbischof changed the title Move Bert to applications folder Move Bert to models/ folder Aug 10, 2022
@jbischof jbischof changed the title Move Bert to models/ folder Move Bert to models folder Aug 10, 2022
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Left a more detailed pass of comments!

Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Few more comments.

@mattdangerw
Copy link
Member

mattdangerw commented Aug 11, 2022

Axes we should test this on (though need not be all on this PR):

  1. Directly calling a model, directly calling a model with a classification head (maybe just a shape assertion on output).
  2. Compiling a classification model with a loss and running a very small fit() (e.g. two batches of two examples).
  3. Doing the 2) but compiling the model with jit_compile=True.
  4. Saving a model with model.save(some path), restoring with model.load() and asserting the outputs for original and saved are verbatim equal.

This PR has an example of how to test with and without jit_compile (with XLA and without XLA) in a parameterized test.
https://github.com/keras-team/keras-nlp/pull/271/files

We can figure out weight loading tests, and what (if any) correctness testing we want to do in unit testing after we actually have some checkpoints to load.

@jbischof
Copy link
Contributor Author

jbischof commented Aug 12, 2022

I added a docstring example usage for keras_nlp.models.Bert(), but this doesn't appear to be tested automatically. I verified in colab.

@jbischof jbischof marked this pull request as ready for review August 12, 2022 03:48
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Looking good, last few comments on this round of the API I think!

@mattdangerw
Copy link
Member

@jbischof re docstrings.

The >>> style docstring will get tested. The fenced style docstring will not, but IMO they are still much more readable for larger code blocks. We could work to add fenced docstring testing, tf core has some code for it here, but no project is actually using that yet.

Anyway, most of the testing should be on the unit tests. That's just a good way to make sure our docstring don't get stale.

Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Another couple spots I just noticed!

@mattdangerw
Copy link
Member

Axes we should test this on (though need not be all on this PR):

  1. Directly calling a model, directly calling a model with a classification head (maybe just a shape assertion on output).
  2. Compiling a classification model with a loss and running a very small fit() (e.g. two batches of two examples).
  3. Doing the 2) but compiling the model with jit_compile=True.
  4. Saving a model with model.save(some path), restoring with model.load() and asserting the outputs for original and saved are verbatim equal.

Of these, I think 1. and 4. are probably the ones we should make sure to have on this PR. @chenmoneygithub are you OK to review the testing code on this when it is ready (probably quite shortly)?

Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

LGTM from me! With the understanding that testing will come in here after I'm on vacay.

Thanks for the huge amount of work here. This is big!!

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

Successfully merging this pull request may close these issues.

4 participants