-
Notifications
You must be signed in to change notification settings - Fork 309
Add AlbertClassifier
#668
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
Add AlbertClassifier
#668
Conversation
|
@jbischof This PR is ready for review. Please consider following points while reviewing
|
|
@jbischof @mattdangerw This PR is ready for review as well ! |
|
CI is green @jbischof |
mattdangerw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Left a few comments, mostly because we have made some changes to base classes here.
|
@mattdangerw @jbischof Couple of notes about following commit.
|
|
@jbischof This PR is ready for review. |
mattdangerw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Just some small issues. There are some docs changes we will need, but I think we can leave that to a follow up.
You still have some formatting issues, make sure to run our formatting script.
| Examples: | ||
| ```python | ||
| # Call classifier on the inputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to add some more documentation for overall usages here, see BERT as an example -> https://github.com/keras-team/keras-nlp/blob/master/keras_nlp/models/bert/bert_classifier.py#L55-L161
But I'm also OK with this being a follow up, we have some work to do on our class level docstrings anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just do this one as a follow up, I've opened #688. Other comments still apply!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made current docstrings alone with Bert classifier, once the pr merges and we have clear cut idea on how the docstrings for all models are to be restructured, I can put a follow up PR.
|
Still WIP |
|
merging with master messed up albert_presets.py changes. fixed it. Ready for review @mattdangerw @jbischof I'm not sure why nightly is fialing in CI |
|
CI is almost green. |
|
Thanks so much @shivance ! Not sure what is going on with nightly, but I suspect it is unrelated to your PR. Nightly testing is important for us to have, but it will definitely surface errors in the upstream tf release as often as our own. I will take a closer look today. Thanks! |
|
@mattdangerw How does nightly differ from dev and stable release? |
|
@shivance only difference is that nightly will test against the tf-nightly and tensorflow-text-nightly packages. Which are nightly "unstable" releases of our upstream dependencies. Often what will happen is the |
|
Got it ! Thanks ! @mattdangerw What are potential projects ? (My guess is, ideally it should align with the current roadmap of KerasNLP) |
I'm pretty sure yes! But we are just figuring out all the details right now actually, will follow up when we have more. Also, confirmed that this nightly test is unrelated to your change. Will pull this in tomorrow, and thank you! |
This PR adds AlbertClassifier class to keras_nlp, letting the user instantiate the classifier with AlbertaBackbone, out-of-the-box.
Closes #660 !