Skip to content

Conversation

kanpuriyanawab
Copy link
Collaborator

Fixes #823

Needs @mattdangerw 's confirmation, keeping this comment in mind.

@mattdangerw
Copy link
Member

Actually Sampler and Tokenizer we do want to export right now, as we want library users to be able to write subclasses for these.

This looks good for Backbone, Task, and Preprocessor!

@kanpuriyanawab
Copy link
Collaborator Author

Hey @mattdangerw, changes us are up, and PR is RTM !
btw could you quickly summarize how keras_nlp_export differs from register_keras_serializable in terms of functioning, (apart from export logic).
Thanks !

@mattdangerw
Copy link
Member

Sure! keras_nlp_export just does register_keras_serializable under the hood. https://github.com/keras-team/keras-nlp/blob/master/keras_nlp/api_export.py

Generally it is good practice to make sure every object in the library is registered as serializable if it (or a parent class) has a from_config(). So that is basically all layers, tokenizers, models, metrics, samplers, etc. Whether or not they are part of the API.

keras_nlp_export means we are adding this to the API of the library. Everything with an export annotation will be under our API contract and documented on keras.io. We have things (like the WhisperEncoder we just added) that we don't want to export and are essentially private details of our library. This includes Task and Backbone classes for now. We don't want people writing their own subclasses and filing bugs if we break compatibility on these classes.

So basically all "keras objects" should have either an register call or an export call. Register if its private functionality, export if its public.

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! Will merge as soon as CI is green.

@mattdangerw mattdangerw merged commit b2df61a into keras-team:master Mar 10, 2023
@kanpuriyanawab kanpuriyanawab deleted the api_export_base_class branch May 21, 2023 15:13
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.

Remove API export decorator from base classes
3 participants