Skip to content
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

BertMaskedLM Task Model and Preprocessor #774

Merged
merged 22 commits into from
Mar 3, 2023

Conversation

Cyber-Machine
Copy link
Contributor

@Cyber-Machine Cyber-Machine commented Feb 23, 2023

Fixes #719
I have made the following changes, but I am still, working on the process:

  • Update BertTokenizer to expect a mask token.
  • Add a BertMaskedLMPreprocessor preprocessor layer and tests.
  • Add a BertMaskedLM task model and tests.
  • Update keras_nlp/models/__init__.py to export BertMaskedLM and BertMaskedLMPreprocessor.

@Cyber-Machine Cyber-Machine marked this pull request as draft February 23, 2023 19:00
@Cyber-Machine
Copy link
Contributor Author

@mattdangerw This PR is ready for review.
Here's the gist of the working model.

@Cyber-Machine Cyber-Machine marked this pull request as ready for review February 24, 2023 19:55
@Cyber-Machine Cyber-Machine changed the title WIP: BertMaskedLM Task Model and Preprocessor BertMaskedLM Task Model and Preprocessor Feb 24, 2023
@shivance
Copy link
Collaborator

@Cyber-Machine use black to format your code, before making a commit, to format your code.

keras_nlp/models/bert/bert_masked_lm_test.py Show resolved Hide resolved
keras_nlp/models/bert/bert_masked_lm.py Outdated Show resolved Hide resolved
keras_nlp/layers/masked_lm_mask_generator.py Outdated Show resolved Hide resolved
@Cyber-Machine
Copy link
Contributor Author

@mattdangerw @abheesht17 This PR is ready for review.

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.

Thank you! This looks great to me. Just some minor comments.

@keras.utils.register_keras_serializable(package="keras_nlp")
class BertMaskedLMPreprocessor(BertPreprocessor):
"""BERT preprocessing for the masked language modeling task.
This preprocessing layer will prepare inputs for a masked language modeling
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the empty newlines from the version you copied from got removed. (github does this for some reason)

Can you add them back in throughout this docstring?

self.assertAllEqual(
x["padding_mask"],
[
True,
Copy link
Member

Choose a reason for hiding this comment

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

If possible, shorted these examples so we don't take up so much vertical space here.

You should be able to pass 0s and 1s here which should help, we do that for other tests.

Copy link
Collaborator

@abheesht17 abheesht17 left a comment

Choose a reason for hiding this comment

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

Couple of NITs

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""BERT masked lm model."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

"lm" --> "LM"

intermediate_dim=3072,
max_sequence_length=12
)
# Create a BERT masked_lm and fit the data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Crete a BERT masked LM model and fit the data."

@mattdangerw
Copy link
Member

Thank you! This is great!

@mattdangerw mattdangerw merged commit 91fe6bd into keras-team:master Mar 3, 2023
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.

Add a BertMaskedLM task model
4 participants