Skip to content

Moved test out of run_in_graph_and_eager_mode in triplet_test #1523

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
Merged
merged 4 commits into from
Apr 2, 2020
Merged

Moved test out of run_in_graph_and_eager_mode in triplet_test #1523

merged 4 commits into from
Apr 2, 2020

Conversation

autoih
Copy link
Member

@autoih autoih commented Apr 1, 2020

@boring-cyborg boring-cyborg bot added the losses label Apr 1, 2020
@bot-of-gabrieldemarmiesse
Copy link

@lc0

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

@autoih
Copy link
Member Author

autoih commented Apr 1, 2020

@gabrieldemarmiesse, I'm not sure why having issue related to bazel, ... Use the --test_verbose_timeout_warnings command line option to see which ones these are. Do you have any suggestions?

@gabrieldemarmiesse
Copy link
Member

If you scroll in the logs, you'll see the stacktrace. It looks like a flaky test. We should have increased the absolute and relative tolerance to 1e-6. Also be aware that the build of the master branch is currently broken because we use too much disk space. So you may experience failed builds because of that until we fix it.

@lc0
Copy link
Contributor

lc0 commented Apr 1, 2020

The change itself looks reasonable to me. Thanks for updating @autoih

Just to cross link to original one - #1328

@gabrieldemarmiesse gabrieldemarmiesse self-assigned this Apr 1, 2020
gabrieldemarmiesse
gabrieldemarmiesse previously approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse 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, thanks again! You've been a great help! We just need to wait for the fixes to land in the master branch.

@gabrieldemarmiesse
Copy link
Member

And thanks @lc0 for the review!

@googlebot

This comment has been minimized.

Sign in to view
@googlebot googlebot added cla: no and removed cla: yes labels Apr 1, 2020
@gabrieldemarmiesse
Copy link
Member

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 1, 2020
@seanpmorgan
Merge branch 'master' into pr/autoih/1523
6e1b974 
# Conflicts:
#	tensorflow_addons/losses/triplet_test.py
@googlebot

This comment has been minimized.

Sign in to view
@googlebot googlebot added cla: no and removed cla: yes labels Apr 2, 2020
@seanpmorgan
Copy link
Member

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 2, 2020
seanpmorgan
seanpmorgan approved these changes Apr 2, 2020
View reviewed changes
Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

LGTM thanks again!

@seanpmorgan seanpmorgan merged commit 5958875 into tensorflow:master Apr 2, 2020
@autoih autoih deleted the ptl32 branch April 2, 2020 22:17
jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
@autoih @gabrieldemarmiesse @seanpmorgan
Moved test out of run_in_graph_and_eager_mode in triplet_test (tensor…
8fbee26 
…flow#1523)

* Moved test out of run_in_graph_and_eager_mode in triplet_test

* set tolerance

Co-authored-by: gabrieldemarmiesse <[email protected]>
Co-authored-by: Sean Morgan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanpmorgan seanpmorgan seanpmorgan approved these changes

@gabrieldemarmiesse gabrieldemarmiesse gabrieldemarmiesse left review comments

Assignees

@gabrieldemarmiesse gabrieldemarmiesse

Labels
cla: yes losses
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@autoih @bot-of-gabrieldemarmiesse @gabrieldemarmiesse @lc0 @googlebot @seanpmorgan