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

Jump to bottom

FIx random gamma and space to depth tests. #50813

Merged
merged 1 commit into from Sep 14, 2021
Merged

FIx random gamma and space to depth tests. #50813

merged 1 commit into from Sep 14, 2021

Conversation

puneeshkhanna
Copy link
Contributor

@puneeshkhanna puneeshkhanna commented Jul 17, 2021 •
edited

Fix random gamma tests to use deprecated v1 graph mode so
that test is executed in both CPU and accelerator because in eager
mode, same device will be be only selected irrespective of use_gpu
flag. Remove loop of use_gpu where not required.
Space to depth testBatchSize0 verification fixed with common
code.

Signed-off-by: puneeshkhanna puneesh.khanna83@gmail.com

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Jul 17, 2021
@google-cla google-cla bot added the cla: yes label Jul 17, 2021
@puneeshkhanna
Copy link
Contributor Author

Please review !

@puneeshkhanna
Copy link
Contributor Author

Also on a separate note, can someone guide me to correct forum or people with whom I can discuss about the usage of use_gpu flag in tensorflow/python/framework/test_util.py.

@gbaned gbaned self-assigned this Jul 19, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jul 19, 2021
@gbaned gbaned added the awaiting review Pull request awaiting review label Jul 23, 2021
@mihaimaruseac mihaimaruseac removed their request for review July 30, 2021 18:01
@puneeshkhanna
Copy link
Contributor Author

Any updates on review ? Can this be merged ?

@gbaned gbaned requested a review from brianwa84 August 9, 2021 15:22
@puneeshkhanna
Copy link
Contributor Author

Any updates ?

@gbaned gbaned requested a review from mdanatg September 1, 2021 14:31
@mdanatg mdanatg requested a review from sanjoy September 1, 2021 14:33
@mdanatg
Copy link

mdanatg commented Sep 1, 2021

Regarding use_gpu, I recommend not using it at all (that means you modifications look good here) - the test suites run on CPU and GPU separately, and in general the same code should produce identical results, irrespective of what device it runs on.

@puneeshkhanna
Copy link
Contributor Author

Regarding use_gpu, I recommend not using it at all (that means you modifications look good here) - the test suites run on CPU and GPU separately, and in general the same code should produce identical results, irrespective of what device it runs on. ->thanks for clearing this. There are so many test cases where a loop of use_gpu of False, True is executed unnecessarily. We can avoid those loops in so many other places un less there is a requirement to compare CPU results vs GPU results. I can start by updating this random gamma test file. I will send updated patch soon. Also I think that it does make sense to have a test comparing random operators behavior between CPU and any other device because random number generator algorithm can be different.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Sep 4, 2021
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Sep 4, 2021
@puneeshkhanna
Copy link
Contributor Author

Hi @mdanatg - Updated files ready for review. Avoided unnecessary loop of use_gpu where not required and the current code will run all the tests on whatever device is connected.
Regarding testCPUGPUMatch specific test - for now lets have @test_util.run_deprecated_v1 for now because graph mode will correctly constrain devices between CPU (when use_gpu is False) and any other device (when use_gpu is True). Moving to tf function seems to be breaking other tests. But will try to take that up in another pull request in near future.

@puneeshkhanna
Copy link
Contributor Author

Btw on same lines, I will try to raise some more pull requests soon where we can avoid unnecessary loops of use_gpu of False, True. It will further speed up the execution time overall.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Sep 8, 2021
@puneeshkhanna
Copy link
Contributor Author

Any updates ? Can this be merged now ? Locally ran all the tests on CPU too and they are passing after all the changes.

mdanatg
mdanatg approved these changes Sep 8, 2021
View reviewed changes
Copy link

@mdanatg mdanatg left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, this slipped through the cracks.

I'm curious about the error you get when using tf.function - happy to help debug it down. But no need to block this PR on that. I forgot to double check, are you using e.g. self.evaluate(diff) instead of diff.eval(). The former is expected to work.

@puneeshkhanna
Copy link
Contributor Author

Sorry for the delay, this slipped through the cracks.

I'm curious about the error you get when using tf.function - happy to help debug it down. But no need to block this PR on that. I forgot to double check, are you using e.g. self.evaluate(diff) instead of diff.eval(). The former is expected to work.

Yes got that but didn't want to confuse tf function with the intend of this pull request. Will surely take it up soon. Hope you saw the changes which will make the execution faster too of the tests by ensuring to not run the test twice where not required. How will this be merged now ?

PR Queue automation moved this from Approved by Reviewer to Closed/Rejected Sep 9, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Sep 9, 2021
@puneeshkhanna puneeshkhanna reopened this Sep 9, 2021
PR Queue automation moved this from Closed/Rejected to Assigned Reviewer Sep 9, 2021
@puneeshkhanna
Copy link
Contributor Author

@mdanatg - I got it what needs to be done for tf function. But didn't want to mix those changes with the intend of this pull request. Will surely take it up in near future. Hope you saw the changes to avoid execution of same test twice and test execution time will be further faster now. How will this be merged now ?

mdanatg
mdanatg suggested changes Sep 9, 2021
View reviewed changes
Copy link

@mdanatg mdanatg left a comment

Choose a reason for hiding this comment

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

Just noticed a small nit; the external linter didn't seem to catch it.

tensorflow/python/kernel_tests/random/random_gamma_test.py Outdated Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Sep 9, 2021
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Sep 9, 2021
@puneeshkhanna
Fix random gamma and space to depth tests.
3e79ac2 
Fix random gamma tests to use deprecated v1 graph mode so
that test is executed in both CPU and accelerator. In eager
mode, only highest priority same device will be selected.
Avoid unnecessary loop of use_gpu wherever applicable.
Space to depth testBatchSize0 verification fixed with common
code. Also update common code of space to depth tests.

Signed-off-by: puneeshkhanna <puneesh.khanna83@gmail.com>
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 9, 2021
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Sep 9, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 9, 2021
@mdanatg
Copy link

mdanatg commented Sep 9, 2021

Thanks, we just need to wait for the merge process to complete now.

@gbaned gbaned removed the awaiting review Pull request awaiting review label Sep 10, 2021
@puneeshkhanna
Copy link
Contributor Author

Some checks were not successful
13 successful and 1 errored checks
@mdanatg - can you please help ? I am not sure what is this error.

@mdanatg mdanatg added the kokoro:force-run Tests on submitted change label Sep 13, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 13, 2021
@mdanatg
Copy link

mdanatg commented Sep 13, 2021

Looks like a tool failure, I started a retry - @gbaned might be able to help getting it unstuck.

@copybara-service copybara-service bot merged commit 15829de into tensorflow:master Sep 14, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Sep 14, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Sep 14, 2021
@gbaned
Copy link
Contributor

gbaned commented Sep 14, 2021

Looks like a tool failure, I started a retry - @gbaned might be able to help getting it unstuck.

@mdanatg, @puneeshkhanna This PR is merged. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdanatg mdanatg mdanatg approved these changes

@brianwa84 brianwa84 Awaiting requested review from brianwa84

@srvasude srvasude Awaiting requested review from srvasude

@sanjoy sanjoy Awaiting requested review from sanjoy

Assignees

@gbaned gbaned

Labels
cla: yes size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@puneeshkhanna @mdanatg @gbaned @kokoro-team @tensorflowbutler