Please use self.assertRaisesRegex to check the exception message.

I think that this function, and the previous two private functions that it calls, should be left in their original location in sparse_tensor_dense_matmul_op_test.py. They should be kept with the code that calls them, to minimize complexity. I don't see any reason to put them in the base file and make them available to the deterministic tests.

Change this to 2015-2021 after moving the benchmark code back in here.

This check should not be run here, because it will run on either a CPU or a GPU. You need to find a place in the codepath (in the CUDA kernel launch code, and not inside the CUDA kernel) where the code will definitely only be running on a GPU.

It seems as though you've added the integral type check to prevent exceptions being thrown on CPU. This will not cover the case where the op is forced to run on CPU, and the type is float64 or complex128, which presumably do operate deterministically on the CPU. Therefore this code will throw incorrect exceptions on CPU.

By making this check GPU-only, it will simplify it, and reduce complexity.

It's a TF standard to put OP_REQUIRES in the *.cc file instead of *.cu.cc otherwise compiling error occurs. Reason is, the functor::compute in *.cu.cc returns a Status but compute in *.cc returns void. If there is a OP_REQUIRES branch in the functor::compute, a return value is mandate.

It is possible, depending on the way the op is coded, to use OP_REQUIRES and OP_REQUIRES_ASYNC in the CUDA kernel launch code in the *.cu.cc file, and some ops do this. It's also possible to add the error to the context, as some other ops do. If, as in this particular op, the signature of the per-device implementation functor returns Status, then it suggests that we need to work with that pattern (set an error as well as, or via, returning a Status). The objectives have to be met while working within reasonable constraints, but we should not default to introducing bugs, reducing performance significantly, or introducing unnecessary complexity because one apparent pattern seems to conflict with another apparent pattern. It's sometimes necessary to wrestle a little bit longer with this kind of thing and/or seek consultation in order to meet the requirements of what needs to be implemented.

One part-way solution, could be to put the OP_REQUIRES into the existing if (std::is_same<Device, GPUDevice>::value) block in the *.cc file; this would simplify and accelerate. The RequireDeterminism function could be defined on the *.cu.cc file, declared in a shared header, and reused. However, I think it's better (simpler/faster) to check for RequireDeterminism() once, in one place (in the *.cu.cc file), assuming a good way to do that can be figured out.

It looks like maybe we just have to return errors:Unimplemented(...).

Looking at, for example, barrier_ops.cc, that contains functions which return Status and throw exceptions.

When the status is passed up the stack like that, a higher level handles it (with OP_REQUIRES_OK), putting it into the context. If it's not passed up (as in functions that return void) the OP_REQUIRES puts it into the context.

In summary: we either throw the exception or we pass it up via returned Status.

Follow-up from phone conversation with @wenscarl: I had missed (the context) that this code was already inside the if (std::is_same<Device, GPUDevice>::value) block, so the "part-way" solution, that I mentioned above, is how it already was.

This should not be checked every time the op compute runs. Please follow the existing pattern by caching the environment variable in a static variable inside a function.

Given that this check will/should move down into the _gpu.cu.cc code, I don't think you'll need to do it twice, you can do the exception check/throw in the same place as choosing the deterministic functionality. So this pattern here should be eliminated.

The integral check should be eliminated.

In the kernel-launching code, in the RequireDeterminism() == true branch, there are two options: (1) if std::is_same<T, double>::value || std::is_same<T, complex128>::value then throw the exception, else (2) do the deterministic kernel launch.

That should not need to be put in a function or inlined, and I think it's going to be really easy to understand.

It's not clear to me if the compiler's default assumption of pointer aliasing could slow down the operation in these DownCast functions (but it's better to be safe and to let it know that there is no pointer aliasing), however, to avoid pointer aliasing, doesn't __restrict__ have to be applied to both src and dst? I just reviewed this blog post (again).

Do you have a reason for not checking that all the words/characters in the message are correct?

To me, it seems better to check it completely. If the message changes, either intentionally or accidentally, this test will then fail, and it will bring conscious attention to this being tested.

Note that self.assertRaisesRegex can test against a regular expression (as you have used it), but it can also be used to compare against a string literal (a type of non-variable regular expression).

I think that this variable name should be more explicit because in the context of this test code, "unimplemented types" means that matmul is not implemented at all for those types, which is not what this variable represents. What this variable is capturing is the types for which GPU-determinism is not implemented. Therefore, an unambiguous name for it would be gpu_determinism_unimplemented_types, otherwise its meaning can only be discerned by carefully examining how it's later used, and, ideally, we want to be able to read and understand code without having to decipher variable meaning by mentally simulating variable use.

Variables should be named such that, in the contexts in which they appear, the meaning of their contents is unambiguous.

Please refer to this comment in my previous review cycle. These two functions are essentially subroutines of the benchmark function, which is not referenced in the base file or the deterministic test file at all. These functions should be returned to their original locations in sparse_tensor_dense_matmul_op_test.py.

Please refer to section 3.2 (Line Length) of the Google Python Style Guide: "Do not use backslash line continuation except for with statements requiring three or more context managers. Make use of Python’s implicit line joining inside parentheses, brackets and braces. If necessary, you can add an extra pair of parentheses around an expression."

There is another instance of incorrect use of backslash for line continuation in the code below.

Also this variable name includes an incorrect spelling of "determinism." Please call it, determinism_required.

I think this line of code should be implemented with multiples of two (not four) spaces: outer=2, inner=4.

This block of code should be indented with four spaces, not five.

This block of code should be indented with four spaces, not three.

Please will you review all the code that you're adding and make sure that it follows the intending rules. Each level of statement indent should add two spaces. Starting a list of parameters on a new line [ending the previous line with a "("] should indent four spaces.

It might seem pedantic, but it's important to generate code in a standard way to make it easy to read and maintain. Search for, and review, all 30 instances of the word "indent" in the Google C++ Style Guide.

I don't think these three blank lines should be removed from the copyright header. They're part of the standard copyright header format/structure.

There should always be a space between the ) and the {. See here. I don't know if these kind of formatting issues are currently auto-fixed, break a CI lint check, or just slip through, but I think it's worth fixing them up-front. Please check that this formatting error is not repeated elsewhere in your PR. I see it in the next function definition, for example.

Note to self: this has been addressed.

Nice catch. I'm wondering how this got into the PR in the first place.

Function names should start with a capital letter; see here.

Reduction is a relatively generic and non-descriptive noun. What does this function actually do? It re-casts and then adds. Following the existing name, how about GpuRecastAtomicAdd? This function recast-adds.

Note to self: this has been addressed.

I notice that you haven't replaced the call to GpuAtomicAdd earlier in this function. Do you have a reason for or against that?

The earlier GpuAtomicAdd doesn't need to do the recast because the template passed in is Tsum already.

I see and understand that now. Thank you.

I wonder why this code is using this slower form of zeroing. Perhaps this should be made to run faster (?) by using SetZero (as in the other branch of the conditional).

Maybe just nobody catches that. Fixed it.

Note to self: this has now been addressed.

Isn't this line of code basically the same as our DownCast function, but possibly slower?

Isn't the only difference between the deterministic and nondeterministic codepaths in the choice of Tsum?

Could this code be re-written more simply to run exactly the same way for both regular and deterministic operation except that for regular operation, Tsum = typename SumType<T>::type while for deterministic operation, Tsum = typename SumType<T>::type_for_determinism?

Maybe I'm missing some other difference between the two code paths.

Echo my earlier thought. This line of code is designed to handle fp16 particularly. For complex type, temp_out_t.matrix<Tsum>().template cast<T>() cannot handle properly and has to go through our DownCast kernel. But we do can uniform the definition of Tsum.

Right. The DownCast kernel will perform the same functionality, and more, and with higher performance (we believe). My question is why not replace this line with a DownCast launch.

The larger point here is that both the regular and deterministic paths in SparseTensorDenseMatMulFunctor do the same thing. The only difference is the choice of Tsum. I'm wondering why these two codepaths cannot be merged into one codepath. As it stands, it looks like the same codepath replicated.

Merging the code paths requires some careful thought, of course, but it seems doable.

The regular codepath handles Tsum == T, which is fine. This goes beyond what the deterministic codepath needs. In the implemented deterministic modes, Tsum != T, and the regular codepath handles that case too. It seems that the regular codepath can be augmented very slightly to support the deterministic functionality as well. Let me know if you would like to discuss and I can go through this with you.

To limit the amount of determinism-related stuff in this base test, you could replace len(gpus) > 0 and determinism_required with self.__class__ == SparseTensorDenseMatmulDeterministicTest. This will remove the requirement for the three lines of code above that set determinism_required and will make this base-test code agnostic to how determinism is enabled (or if it's enabled at all).

Also (np.float64, no.complex128) can be replaced with self._getGpuDeterminismUnimplementedTypes(), a private function, defined only in the SparseTensorDenseMatmulDeterministicTest class, which returns (np.float64, no.complex128) and can also be used by (called from) testDeterministicSparseDenseMatmul in SparseTensorDenseMatmulDeterministicTest. This will result in the list of unimplemented types will only be defined once, in one place.

Really good suggestion.

Thanks.

This string could be returned by self._getGpuDeterminismUnimplementedErrorString(), a private function, defined only in the SparseTensorDenseMatmulDeterministicTest class that returns the string and can also be used by (and called from) testDeterministicSparseDenseMatmul in SparseTensorDenseMatmulDeterministicTest. This will result in the string only being defined once, in one place.

Note to self: this has now been addressed.

This code will never test the exception cases. We want to test implemented and unimplemented types.

Note to self: this has now been addressed.