-
Notifications
You must be signed in to change notification settings - Fork 613
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
Function that print each function's execution time #1174
Conversation
f53e6c6
to
d98a4ad
Compare
With pytest, you don't need special code (that's kind of why I'm advocating to run the tests with pytest, no bazel, it's way more futureproof). For example, to profile the pytest -v --durations=10 tensorflow_addons/optimizers/stochastic_weight_averaging_test.py and I get:
But for the moment, since we're still going to work with bazel for a while, it's better to have tools to help us time the tests. Thanks for the pull request! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! We're going to get a super fast CI at this rate! 😃
Woah, did not know you could do that with |
And that's nothing, just wait until you see #677 😄 |
Alright, so I have good news. I think we can get pytest working with bazel. We'll need to define our own bazel rule (say |
Or we could just use bazel only to build the SO files and use pytest for everything else. It would require nearly no changes to the repo since pytest is already compatible with unittest. We could also get rid of the BUILD files for python if we decide to go with the native python wheels builder. It would mean less work for you. |
But what I really like about Bazel is the incremental and dependent building/testing it offers. If you're working in a python only world, then going with pytest would be A+, but since we do cross-language calls (Python->C++), Bazel is very useful since it can figure out what to recompile and whatnot, this wouldn't be available in pytest. If we ever decide to completely stop all C++/CUDA code (which depends on the progress of XLA and MLIR) then we can drop bazel all together. |
Even if we keep C++/Cuda code, I'm not sure running the tests with bazel is our best option. I believe we need some discussion regarding all of this before implementing anything. Let's open an issue and we'll be able to discuss it there :) |
This came in super handy for #1173 and would be helpful while tackling #1143.
I can close this PR if you don't think it needs to be included in the codebase.