Adds linalg.det alias, fixes outer alias, updates alias testing #42802
Conversation
mruberry
requested review from
apaszke,
ebetica,
goldsborough and
yf225
as code owners
facebook-github-bot
added
the
oncall: jit
💊 CI failures summary and remediationsAs of commit 38afbba (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 16 times. |
mruberry
added
module: tests
test/test_op_normalization.py
Outdated
| is_inplace = info.alias_name.endswith('_') | ||
|
|
||
| # NOTE: this workaround necessary to satisfy the JIT, which | ||
| # does allow Python aliasing or direct calling of torch.Tensor |
test/test_op_normalization.py
Outdated
| script = fn_template.format(alias_name=info.alias_name, args=arg_string) | ||
| else: | ||
| # Checks that scripting converts aliases | ||
| # NOTE: scripting doesn't support splatting args, so this |
There was a problem hiding this comment.
Move this note up since splatting happens in both cases
| call = get_call(method_name, 'method', actuals, kwargs) | ||
| script = script_template.format(', '.join(formals), call) | ||
| CU = torch.jit.CompilationUnit(script) | ||
| torch._C._jit_check_alias_annotation(CU.the_method.graph, tuple(tensors), method_name) |
There was a problem hiding this comment.
this is no longer being called - it's not checking op normalization of aliasing, it's checking alias annotations of schemas
There was a problem hiding this comment.
Aha! I wanted to check on that. I'll revert this change.
| self.decorators = decorators | ||
|
|
||
| alias_infos = ( | ||
| AliasInfo('absolute', torch.absolute, 'abs', |
There was a problem hiding this comment.
I think it's kind of nice to reuse the op registry instead of creating a new one...
There was a problem hiding this comment.
It is! But check this out: #41662.
So now we can update this to use the fancy new op registry.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This PR:
The torch.linalg.outer alias was put the linalg namespace erroneously as a placeholder since it's a "linear algebra op" according to NumPy but is actually still in the main NumPy namespace.
The updates to test_op_normalization are necessary. Previously it was using method_tests to generate tests, and method_tests assumes test suites using it also use the device generic framework, which test_op_normalization did not. For example, some ops require decorators like
skipCPUIfNoLapack, which only works in device generic test classes. Moving test_op_normalization to the device generic framework also lets these tests run on CPU and CUDA.Continued reliance on method_tests() is excessive since the test suite is only interested in testing aliasing, and a simpler and more readable
AliasInfoclass is used for the required information. An example impedance mismatch between method_tests and the new tests, for example, was how to handle ops in namespaces like torch.linalg.det. In the future this information will likely be folded into a common 'OpInfo' registry in the test suite.The actual tests performed are similar to what they were previously: a scripted and traced version of the op is run and the test verifies that both graphs do not contain the alias name and do contain the aliased name.
The guidance for adding an alias has been updated accordingly.
cc @mattip
Note:
@ngimel suggests:
torch.gernametorch.outer