-
Notifications
You must be signed in to change notification settings - Fork 248
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
Update docs for upcoming 0.2.0 release #158
Conversation
Also, note that this is currently cross linking the keras.io docs, so we should land this after we land the new keras.io section (or I can just ditch those links and submit them later). |
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.
thanks for the documentation work!
1864b80
to
c87453b
Compare
'`pip install rouge-score`.') | ||
``` | ||
|
||
## Keep computation inside TensorFlow graph |
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.
Maybe worth pointing out the difference between "TF graph" (tf.function
) and "XLA compilable" (tf.function(jit_compile=True)
)
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.
Good point.
Though I fuzzy on what we should suggest here. I'm going to say we want both, but for preprocessing and postprocessing, XLA is not a requirement. I don't think you can use the tf text custom ops with XLA.
self.units = units | ||
|
||
def build(self, input_shape): | ||
super().build(input_shape) |
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.
Is the super call necessary?
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 have been unsure about this for a while! Core keras has two styles...
super().build(input_shape)
or self.built = True
You should have one of those lines. Is either more canonical?
self.w = self.add_weight(shape=(input_shape[-1], self.units)) | ||
self.b = self.add_weight(shape=(self.units,)) | ||
|
||
def call(self, 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.
Might be worth documenting mask
and possibly training
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 is the style guide, so will keep discussion of how to actually handle masking out of here. But added a not that masking behavior of the layer should be documented in the class level docstring.
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.
LGTM, thank you!
reduce the friction of training a tokenizer, we should offer a standardized | ||
experience for training new vocabularies. | ||
|
||
### Scalable examples of popular model architectures using KerasNLP |
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.
"scalable" -> should we mention DTensor support somewhere? This will be important for model parallelism.
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.
Sounds good. Added a line.
ROADMAP.md
Outdated
to have KerasNLP components available in a pre-trained model offering of some | ||
form. | ||
|
||
We are working with the rest of the Tensorflow ecosystem (e.g. TF Hub, |
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.
TensorFlow
(also we aren't exactly working with TF Hub, are we?)
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.
True, removed.
Submitting this without the keras.io links before I cut 0.2.0. I will push the keras.io links back as soon as the section is live. |
You can browse what this will actually render like here:
https://github.com/mattdangerw/keras-nlp/tree/update-docs