💊 CI failures summary and remediations

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

• 1/1 failures possibly* introduced in this PR
  • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

• Failed: pr/caffe2-pytorch-linux-xenial-rocm3.3-py3.6-test

I didn't follow the revert; what broke, and how does this fix it?

@ezyang yesterday morning conversation about docker images between you and @zou3519:

@zou3519 :

So the conclusion is: your PR may have just changed the compilation speeds of everything so that the clobbering (see Pytorch OSS CI thread) is noticeable

Should be fixed by #39863

@pbelevich merged this pull request in e62d655.

@pbelevich merged this pull request in e62d655.

@ezyang
pytorch extensions require including torch/extension.h which includes all.h which includes utils.h which includes Parallel.h.
So you can call for instance parallel_for() from your extension, but it will fail because of Parallel.h has only declarations and definitions are conditionally included Parallel.h(see last lines of Parallel.h).
I tried to solve it #39612 and #39881 by including Config.h into Parallel.h
But if Pytorch is built with TBB it provides Config.h that has AT_PARALLEL_NATIVE_TBB=1(see #3961 or #39881) and it means that if you include
torch/extension.h which transitively includes Parallel.h which transitively includes tbb.h which is not available! Now the question is: should we provide TBB(and other libs) headers if PyTorch was built with it? Or exclude Parallel.h as a transitive dependency of torch/extension.h ?

@ilia-cher may know better, but assuming you were right in #39471 and if you compile PyTorch with TBB you MUST use TBB implementation of Parallel.h, then I would certainly expect that tbb.h should be available in one way or another if you want to use the Parallel.h functionality.

Now, IIRC, we don't ever distribute binaries that are linked against tbb, so if I understand correctly, your problems here are just because of the TBB CI job. In a perfect universe, we'd have a proper distribution story for TBB, but this would require you to solve a lot more problems (because we'd need to say where tbb is actually coming from; are we relying on a third-party conda package, or distributing our own copy ourselves? What happens with mkl?) So it doesn't seem like the right call is to try to fix the distribution problem for TBB, unless you want to solve this problem for its own sake.

But it also seems likely that you will run into problems with removing Parallel.h from the dependency chain of torch/extension.h, as this would technically be a BC-breaking change, making functionality that was previously implicitly available no longer available. And it's not unreasonable to think that third party C++ extensions will be using the functionality of Parallel.h, since it provides functionality that would be generally useful if you were writing a kernel.

So, if I were in this situation, I'd probably look for some surgical fix for the relevant CI configuration, rather than a general fix. But that is just an opinion.

"if you compile PyTorch with TBB you MUST use TBB implementation of Parallel.h, then I would certainly expect that tbb.h should be available in one way or another if you want to use the Parallel.h functionality."

That's correct, if you set TBB option then we have to use TBB version of Parallel.h and we do use TBB submodule, is this different now?
And yes this option exists for users who want/can build PyTorch themselves.

Are we still running TBB build in CI (master)? @pbelevich @ezyang

Are we still running TBB build in CI (master)? @pbelevich @ezyang

yes, here is the example of tbb/tbb.h: No such file or directory when I tried to land #39881

@ezyang this is my "surgical fix"

That's correct, if you set TBB option then we have to use TBB version of Parallel.h and we do use TBB submodule, is this different now?
And yes this option exists for users who want/can build PyTorch themselves.

Essentially, what I am saying is that the commits that added TBB to PyTorch were incomplete: a complete set of diffs also needs to arrange for the correct TBB headers to be made available to C++ extensions. Since we don't seem to actually distribute official TBB binaries for anyone, it seems reasonable enough to skip doing this today, but if you ever want to actually have people use TBB for real, this is one of the things that still needs to get fixed.

@ezyang sure we can add headers, though regarding your last point, I'd want to note that people (companies) actually do use TBB builds of pytorch (I can provide more detail)

Yeah, in this case I'm specifically talking about our OSS distributions, e.g., the wheels and conda packages.