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.

Sign up for GitHub

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

Jump to bottom

Attempt to fix Flax CI error(s) #8829

Merged
merged 11 commits into from
Nov 30, 2020
Merged

Attempt to fix Flax CI error(s) #8829

merged 11 commits into from
Nov 30, 2020

Conversation

mfuntowicz
Copy link
Member

@mfuntowicz mfuntowicz commented Nov 28, 2020 •
edited

  • Increased the tolerance when comparing Flax et PyTorch output (~0.00058 on my dev box)
  • Removed the jit parametrization when running test_multiple_sentences because it leads to instabilities
  • Introduced subtests expliciting what we're doing by enabling / disabling JIT.

@mfuntowicz
Slightly increase tolerance between pytorch and flax output
40b17eb 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
test_multiple_sentences doesn't require torch
8627da6 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
Simplify parameterization on "jit" to use boolean rather than str
ee9fd5c 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
Use require_torch on test_multiple_sentences because we pull the …
7e4a86e 
…weight from the hub.

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
Rename "jit" parameter to "use_jit" for (hopefully) making it self-do…
ae73677 
…cumenting.

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
Remove pytest.mark.parametrize which seems to fail in some circumstances
6964024 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
Fix unused imports.
4533adf 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
Fix style.
ef6fc3c 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
@mfuntowicz
Give default parameters values for traced model.
0e8ad44 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
patrickvonplaten
patrickvonplaten approved these changes Nov 28, 2020
View reviewed changes
Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

I like it a lot! makes the tests easier to read and seems to fix CI - thanks a lot!

LysandreJik
LysandreJik approved these changes Nov 29, 2020
View reviewed changes
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.

Cool! Thanks for looking into it.

tests/test_modeling_flax_bert.py Outdated Show resolved Hide resolved
@mfuntowicz
Review comment: Change sentences to sequences
895410d 
Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>
sgugger
sgugger approved these changes Nov 30, 2020
View reviewed changes
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 for fixing!

tests/test_modeling_flax_bert.py Outdated Show resolved Hide resolved
tests/test_modeling_flax_roberta.py Outdated Show resolved Hide resolved
@sgugger sgugger merged commit 51b0713 into master Nov 30, 2020
@sgugger sgugger deleted the fix-flax-ci branch November 30, 2020 18:43
stas00 pushed a commit to stas00/transformers that referenced this pull request Dec 5, 2020
@mfuntowicz @sgugger @stas00
Attempt to fix Flax CI error(s) (huggingface#8829)
1bb61f0 
* Slightly increase tolerance between pytorch and flax output

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* test_multiple_sentences doesn't require torch

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Simplify parameterization on "jit" to use boolean rather than str

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Use `require_torch` on `test_multiple_sentences` because we pull the weight from the hub.

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Rename "jit" parameter to "use_jit" for (hopefully) making it self-documenting.

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Remove pytest.mark.parametrize which seems to fail in some circumstances

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Fix unused imports.

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Fix style.

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Give default parameters values for traced model.

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Review comment: Change sentences to sequences

Signed-off-by: Morgan Funtowicz <funtowiczmo@gmail.com>

* Apply suggestions from code review

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

@LysandreJik LysandreJik LysandreJik approved these changes

@sgugger sgugger sgugger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mfuntowicz @patrickvonplaten @LysandreJik @sgugger