Skip to content

Conversation

@WindQAQ
Copy link
Member

@WindQAQ WindQAQ commented May 21, 2019

Fix failed builds because of update in upstream.

@kyleabeauchamp
Copy link
Contributor

Is there a nightly build link or something that shows the failure? I trust you, but for the sake of reproducibility might be nice to have some sort of link to the exception.

Copy link
Contributor

@kyleabeauchamp kyleabeauchamp left a comment

Choose a reason for hiding this comment

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

LGTM

@WindQAQ
Copy link
Member Author

WindQAQ commented May 21, 2019

@kyleabeauchamp
Copy link
Contributor

Thanks!

@WindQAQ
Copy link
Member Author

WindQAQ commented May 21, 2019

Shape checking within tf.function is somewhat not robust in either c++ or python API... @facaiy, hi facai, is there any suggested way to do this?

Btw, thanks for asking and review, Kyle 😄

@seanpmorgan
Copy link
Member

Thanks for the fix! Just trying to understand what happend here... typically the RegEx is supposed to catch the error thrown by this I believe:
https://github.com/tensorflow/addons/blob/master/tensorflow_addons/custom_ops/image/cc/ops/distort_image_ops.cc#L33

But at first pass I can't find the commit that affected this. LGTM to merge if @facaiy is good with it.. but I think something has changed with shape checking in our op?

@seanpmorgan
Copy link
Member

One option would be to merge this fix and create a separate issue to look into why this happend.

@WindQAQ
Copy link
Member Author

WindQAQ commented May 22, 2019

Thanks for the fix! Just trying to understand what happend here... typically the RegEx is supposed to catch the error thrown by this I believe:
https://github.com/tensorflow/addons/blob/master/tensorflow_addons/custom_ops/image/cc/ops/distort_image_ops.cc#L33

But at first pass I can't find the commit that affected this. LGTM to merge if @facaiy is good with it.. but I think something has changed with shape checking in our op?

Thank you, Sean! The original regex text and the type of exception seem wrong while the test passed previously...

OP_REQUIRES(context, input.dims() >= 3,
errors::InvalidArgument("input must be at least 3-D, got shape",
input.shape().DebugString()));

Anyway, because the tests fail randomly, I will create an issue for it.

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.

Approving.. discussion of how this should be done can be solved in #260

@seanpmorgan seanpmorgan merged commit c60fed8 into tensorflow:master May 22, 2019
@WindQAQ WindQAQ deleted the fix-distort-image-ops-test branch May 22, 2019 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants