Add Base Task Class #671
Add Base Task Class #671
Conversation
There was a problem hiding this comment.
Looks amazing @abheesht17 I think we mainly need to beef up the class-level docstrings to include examples with and without preprocessing. Put these up top since it is primary usage!
| { | ||
| "num_classes": self.num_classes, | ||
| "dropout": self.dropout, | ||
| "name": self.name, |
There was a problem hiding this comment.
Could "name" and "trainable" be added to the Task get_config? I guess it would be awkward to look for them in kwargs to pass to super().get_config()?
There was a problem hiding this comment.
We should make this change. I was going to do the same for backbones.
Having name and trainable handle in a base class is actually the norm, so it will improve readability here.
| { | ||
| "num_classes": self.num_classes, | ||
| "dropout": self.dropout, | ||
| "name": self.name, |
There was a problem hiding this comment.
We should make this change. I was going to do the same for backbones.
Having name and trainable handle in a base class is actually the norm, so it will improve readability here.
|
|
||
| model.load_weights(weights) | ||
| return model | ||
| BertClassifier.from_preset.__func__.__doc__ = Task.from_preset.__doc__ |
There was a problem hiding this comment.
We can switch to the approach in #654 now
keras_nlp/models/task.py
Outdated
|
|
||
| @property | ||
| def backbone(self): | ||
| """A `keras_nlp.models.backbone.Backbone` instance providing the encoder submodel.""" |
There was a problem hiding this comment.
Let's make this shorter, not include the "encoder" language (which will not apply to t5, bart, gpt), and not refer to the keras_nlp.models.backbone.Backbone symbol, which we will not expose.
A keras.Model instance providing the backbone submodel.
keras_nlp/models/task.py
Outdated
|
|
||
| @property | ||
| def preprocessor(self): | ||
| """A `keras_nlp.models.preprocessor.Preprocessor` instance for preprocessing inputs.""" |
There was a problem hiding this comment.
Same here.
A keras.layers.Layer instance used to preprocess raw inputs.
In general trying to fit in the 80 char limit is a good constraint for the docstring one liner.
|
/gcbrun |
There was a problem hiding this comment.
This looks really nice! Thank you!
I think as a follow up, I would love to take stock of the full set of code examples we have for classifiers and shorten/rearrange them. Mainly, I want to see from_preset() flows at the top, they are how the vast majority of people will use the class. But let's do that separately!
Resolves #635