💊 CI failures summary and remediations

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

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

@mruberry PTAL. Not sure why that single build is failing.

Also, I have a few questions:

1. When would you implement the in place version of a function, especially for binary functions? The README seemed to indicate that this was legacy and Implement logaddexp #38384 didn't implement it for logaddexp, but Implement gcd, lcm #40651 did implement it for lcm and gcd.
2. When should you implement AVX/Vec256 support for a function? Implement logaddexp #38384 did implement it for logaddexp, but Add arcosh, arcsinh and arctanh to unary ops #38388 did not for arcsinh, arccosh, and arctanh.
3. In the Vec256 headers, when should you use the sleef function vs the _mm256 function? I noticed while most used the sleef function, some used the _mm256 function even though there was an equivalent sleef function (like for sqrt, floor, ceil).

Mainly asking these to try to be consistent. Thanks!

It looks like the build failed with an infrastructure error. Sorry about that. It doesn't indicate anything is wrong with this PR.

1. When would you implement the in place version of a function, especially for binary functions? The README seemed to indicate that this was legacy and Implement logaddexp #38384 didn't implement it for logaddexp, but Implement gcd, lcm #40651 did implement it for lcm and gcd.

We're not very stringent about what is/isn't a method, and it's OK to not implement a method variant of hypot. The README suggests it's "core" operations which are also methods and that functions which are "too complicated" aren't.

2. When should you implement AVX/Vec256 support for a function? Implement logaddexp #38384 did implement it for logaddexp, but Add arcosh, arcsinh and arctanh to unary ops #38388 did not for arcsinh, arccosh, and arctanh.

Adding vectorized CPU support is great and of interest when an operation is part of a performance bottleneck, but not required for the first version of an op (unless it's perf critical).

3. In the Vec256 headers, when should you use the sleef function vs the _mm256 function? I noticed while most used the sleef function, some used the _mm256 function even though there was an equivalent sleef function (like for sqrt, floor, ceil).

That is an excellent question! I think we prefer using the _mm256 where available.

@mruberry PTAL, I made all the requested changes

@mruberry, I added bfloat16 to the tests, but since bfloat16 does not seem to be supported for numpy arrays, I tested against the equivalent formula just for bfloat16. Is that okay, or should I create a different test with hardcoded values just for bfloat16?

@mruberry, I added bfloat16 to the tests, but since bfloat16 does not seem to be supported for numpy arrays, I tested against the equivalent formula just for bfloat16. Is that okay, or should I create a different test with hardcoded values just for bfloat16?

Sounds good. Another option is to compare against fp32 in NumPy.

I'll run this PR through our internal tests now. Let me know if you're interested in implementing another NumPy function.

This is failing on some internal mobile builds:

error: no member named 'hypot' in namespace 'std'; did you mean simply 'hypot'?
      ret[i] = std::hypot(values[i], b[i]);

@dreiss Do you know how we handle mobile issues like this?

I realized that I didn't add hypot to aten/src/ATen/core/aten_interned_strings.h. Could that be the reason why the build is failing?

If not, should I just remove the Vec256 support, and leave that for a later PR?

I realized that I didn't add hypot to aten/src/ATen/core/aten_interned_strings.h. Could that be the reason why the build is failing?

If not, should I just remove the Vec256 support, and leave that for a later PR?

I'm not sure, unfortunately. This is also happening on nextafter, strangely. I'm going to try to follow-up with the mobile team tomorrow but versions of these PRs without the vectorized code paths would be the fastest way to get them in. We can follow-up with the vectorized code paths after the mobile issues are resolved.

Big thank you to @dreiss who took the time to discuss the Android build issues with me offline.

@dreiss suggests trying two things. First, implement hypot (and nextafter) in c10/util/math_compat.h:

Second, add a simple vectorized implementation on Neon that just loops over its inputs and calls std::hypot (std::nextafter). I realize I suggested removing the vectorized Neon you wrote previously since it wasn't required for server builds, so my mistake there. @dreiss suggested looking at the implementations of pow and atan2 in that file as a guide, if needed.

The c10/util/match_compat.h implementations for hypot (and nextafter) don't have to work, just like the nexttoward implementation.

Does that make sense? If that doesn't fix the issue then I'll follow-up with @dreiss and the mobile team again and see if we can help out instead of requesting all these changes from you, @muthuArivoli. We appreciate your patience and diligence dealing with these issues.

@mruberry I added the vectorized implementation back to Neon, and I added the function to the math_compat header. Currently, I'm just throwing an error, but hopefully someone will eventually be able to implement and test that. If this works, I'll go ahead and do the same on the nextafter PR.

Also, seems like the linting error is from someone else's work.

@mruberry I added the vectorized implementation back to Neon, and I added the function to the math_compat header. Currently, I'm just throwing an error, but hopefully someone will eventually be able to implement and test that. If this works, I'll go ahead and do the same on the nextafter PR.

Also, seems like the linting error is from someone else's work.

I'll run the update through the Android builds now.

Android build is fixed. OSS PR build failures look like a failure in the base, not a problem with this revision. Starting the land process now.

Looks like a conflict slipped in. Would you please rebase (and fix the nit, I guess, since the PR must be rebased anyway).

Just fixed the merge conflict and fixed the method signature

@mruberry merged this pull request in 92885eb.