Codecov Report

Merging #45742 into gh/smessmer/258/base will not change coverage.
The diff coverage is n/a.

@@                  Coverage Diff                  @@
##           gh/smessmer/258/base   #45742   +/-   ##
=====================================================
  Coverage                 68.19%   68.19%           
=====================================================
  Files                       410      410           
  Lines                     53399    53399           
=====================================================
  Hits                      36418    36418           
  Misses                    16981    16981           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5c0dbc...3112f3a. Read the comment docs.

Quick meta comment: when we do x is Enum.choice pattern in the codebase, we cannot easily update these occurrences when an enum changes, as the typechecker will not tell you these sites are out of date. If we're likely to keep evolving the number of fields we have in these enum or not, we should think more carefully about how to make sure the type checker can help us in this case. (More detailed guidance after I read over the PR)

OK, I've read over the PR. I don't like how you had to update all of the use_c10_dispatcher sites, because in many cases the update was not actually relevant to the substance of this change. To put it differently: we intend to remove hacky_wrapper_for_legacy_signatures at some point in the future, and so we should engineer the addition of this flag so that it is easy to remove. But as we've written it here, we have to do a lot of unrelated extra codemods to actually eliminate it; that increases risk that we won't properly clean things up later.

Here's what I think: you should have hacky wrapper for legacy signatures controlled by a separate field. Its action on codegen is orthogonal to use_c10_dispatcher. I audited your PR for sites where you didn't either test for local.use_c10_dispatcher() is UseC10Dispatcher.full or local.use_c10_dispatcher() is UseC10Dispatcher.hacky_wrapper_for_legacy_signatures or its inverse local.use_c10_dispatcher() is UseC10Dispatcher.with_codegenerated_unboxing_wrapper, and there are exactly two sites: https://github.com/pytorch/pytorch/pull/45742/files#diff-8dce5c1f99f60f2b38ebffda23c954d8R278 and https://github.com/pytorch/pytorch/pull/45742/files#diff-8dce5c1f99f60f2b38ebffda23c954d8R601 aka the places you actually need to decide whether or not to use the wrapper. All of the other line noise is 100% unnecessary.

If you want to make it illegal to have hacky_wrapper_for_legacy_signatures and non-full c10 dispatch, you can add that as an extra validation step on YAML parsing, or just raise an error in the relevant codegen spots.

Quick meta comment: when we do x is Enum.choice pattern in the codebase, we cannot easily update these occurrences when an enum changes, as the typechecker will not tell you these sites are out of date. If we're likely to keep evolving the number of fields we have in these enum or not, we should think more carefully about how to make sure the type checker can help us in this case. (More detailed guidance after I read over the PR)

That's why I'm adding assertions into the "else" case everywhere. The type checker won't tell you, but if you miss a case, then the assertion will fail. This is actually how I found these if statements I have to change. I don't anticipate adding further values to the enum in the future, but then I didn't originally anticipate to add the hacky_wrapper value, so you never know.

About making hacky_wrapper a separate field from use_c10_dispatcher: I see your point and it indeed is an orthogonal flag if you look at the codegen implementation, but it is not orthogonal if you look at it semantically. I think we should keep it in use_c10_dispatcher since:

• It is semantically a natural progression for the migration of an op to go through the different stages of use_c10_dispatcher. From with_codegenerated_unboxing_wrapper (which is equivalent to use_c10_dispatcher being omitted) to hacky_wrapper_for_legacy_signatures to full. This is linear progress and adding a second dimension doesn't make sense from a semantics point of view.
• As you mentioned, when we add a second flag, we then have to add assertions to make sure that the flags don't contradict each other, i.e. we still have 3 possible states but now use 2 boolean fields to represent them and need to assert against the fourth state. Having to use assertions against invalid states is an indication that the modeling isn't great, it's better to have the invalid state not be representable at all.

Also note that removing the flag is still simple, a first step will be to remove use_c10_dispatcher: with_codegenerated_unboxing_wrapper and remove all the branches for that, then later also remove use_c10_dispatcher: hacky_wrapper_for_legacy_signatures in the same way and only keep the full branches.

If the implementation decisions to use a wrapper or not are orthogonal, then at the very least I would expect a helper function to take UseC10Dispatcher and make the call on each of these orthogonal aspects.

💊 CI failures summary and remediations

As of commit 3112f3a (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This pull request has been merged in 6ba6ecb.