add a specialized check for gradients (RequiresGradCheck) to speed up runtime assumptions check #50392
add a specialized check for gradients (RequiresGradCheck) to speed up runtime assumptions check #50392Krovatkin wants to merge 7 commits intopytorch:masterfrom
Conversation
💊 CI failures summary and remediationsAs of commit 786a2cd539 (more details on the Dr. CI page):
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
bertmaher
left a comment
There was a problem hiding this comment.
Test failures are probably related?
There was a problem hiding this comment.
Just to be sure, this std::vector is only constructed once, right, and then cached inside the lambda below? I just want to make sure we're not allocating on every RequiresGradCheck.
There was a problem hiding this comment.
I don't think rg-props should ever contain c10::nullopt, that wouldn't be checking anything at all. (it also makes it so we have to do an extra unwrap of the bool). Can we change std::vector<c10::optional<bool>> -> std::vector<bool> ?
There was a problem hiding this comment.
Yeah, we can, I just double check that Thomas indeed is conservatively setting requiresGrad to true if we it changed during profiling. Let me update it
There was a problem hiding this comment.
Any reason to check num_inputs > 0 here? Since this is a static property of the RequiresGradCheck op, let's catch it at construction time. (I also have mixed feelings about TORCH_INTERNAL_ASSERT in the interpreter loop period, but I can set that aside for now ;-) )
There was a problem hiding this comment.
err, that was a copy-pasta from TypeCheck! I think I should update both.
There was a problem hiding this comment.
auto t will make a copy of the Tensor. It seems like you should be able to use auto const& t but at least do auto& t.
There was a problem hiding this comment.
ah! a good catch! fixed
There was a problem hiding this comment.
nit: profile (expected) shape is copy-pasta
There was a problem hiding this comment.
just wondering: is there any worry here about undefined tensor? (potentially not) what happens when you call undefined.requires_grad() ?
There was a problem hiding this comment.
undefined tensors shouldn't reach here: even with higher-order derivatives, we either guard AutogradAdd and AutogradAnyNonZero (they become aten::Add) and similar ops or we don't differentiate them?
There was a problem hiding this comment.
why did you add this here ?
There was a problem hiding this comment.
you are right. it doesn't make too much sense since all RequiresGradCheck should be outside :-) or differentiable graphs
facebook-github-bot
left a comment
There was a problem hiding this comment.
@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Codecov Report
@@ Coverage Diff @@
## master #50392 +/- ##
==========================================
- Coverage 80.67% 80.67% -0.01%
==========================================
Files 1910 1910
Lines 207842 207857 +15
==========================================
+ Hits 167684 167689 +5
- Misses 40158 40168 +10 |
786a2cd to
685f51e
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@Krovatkin merged this pull request in 8e60bf9. |
Summary: This change improves perf by 3-4% on fastrnns. Pull Request resolved: pytorch#50392 Reviewed By: izdeby Differential Revision: D25891392 Pulled By: Krovatkin fbshipit-source-id: 44d9b6907d3975742c9d77102fe6a85aab2c08c0
This change replaces general-case TypeChecks on DifferentiableGraphs addded in #49433 with specialized
RequiresGradChecks that only checks if the profiledrequires_gradientsproperties match tensors'requires_gardat runtime.This change improves perf by 3-4% on fastrnns.