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

Keras metric callback #14867

Merged
merged 18 commits into from
Dec 22, 2021
Merged

Keras metric callback #14867

merged 18 commits into from
Dec 22, 2021

Conversation

Rocketknight1
Copy link
Member

@Rocketknight1 Rocketknight1 commented Dec 21, 2021

Hey, here's the unfinished Keras metric PR, co-authored with @merveenoyan!

Still to do:

  1. The outputs from predict/generate are not being concatenated properly, this will be quick to fix! (Edit: Done)
  2. I haven't tested prediction with generate at all. generate() is still very slow, but that's a separate PR 😨
  3. More broad testing with multiple metric functions.

@merveenoyan
Copy link
Contributor

merveenoyan commented Dec 21, 2021

I feel like it makes sense to test on three notebooks. (could be overkill so you decide)

  • Token classification
  • Translation
  • Masked language modeling
    If you approve I’ll test for these ones. I wonder how it would look like with multiple metrics so in one of those I could test with multiple metrics (prolly token classification one)
    @Rocketknight1

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! I think there are a few points to polish but it should be ready to merge soon!

src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
src/transformers/keras_callbacks.py Show resolved Hide resolved
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Very nice! You mentioned that this will only work with the latest version of TensorFlow, should there be a check to ensure that users trying to use this for earlier versions are not surprised when it doesn't work for their setup?

@Rocketknight1
Copy link
Member Author

@LysandreJik Absolutely yes, that's getting added!

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

You'll need to rebase on master and convert all docstrings to Markdown :-)

src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

A few last nits. And I think it's still missing the check for the minimum version of TensorFlow?

src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
src/transformers/keras_callbacks.py Outdated Show resolved Hide resolved
Rocketknight1 and others added 6 commits December 22, 2021 19:21
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@Rocketknight1 Rocketknight1 merged commit b0c7d2e into master Dec 22, 2021
@Rocketknight1 Rocketknight1 deleted the keras_metric_callback branch December 22, 2021 20:35
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.

4 participants