Skip to content

Conversation

@nkovela1
Copy link
Collaborator

This PR replaces all TF ops with Keras ops, making T5 work multi-backend.

@mattdangerw
Copy link
Member

/gcbrun

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.

Looks good to me! Just minor comments.

from keras_nlp.backend import ops


def shape_list(tensor):
Copy link
Member

Choose a reason for hiding this comment

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

we can probably just remove this whole function and use ops.shape where ever it was used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, sorry left over while debugging!

mask_positions,
mask_ids,
) = tf_text.mask_language_model(
(token_ids, mask_positions, mask_ids,) = tf_text.mask_language_model(
Copy link
Member

Choose a reason for hiding this comment

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

this looks like you are using an older version of black maybe?

if you remove the trailing comma it would reformat to one line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed! Yeah just needed to update black

@mattdangerw
Copy link
Member

/gcbrun

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!

@mattdangerw
Copy link
Member

this failure here is xlm-roberta causing an occasional oom. maybe we should just comment that test out for now, it's probably the big embedding eating all available ram. anyway, unrelated

@mattdangerw mattdangerw merged commit 871f664 into keras-team:master Oct 18, 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.

2 participants