@pytorchbot retest this please

Can you explain why setup.py needs to register these serialized tests as data_files? I don't think we want to distribute them as part of the package.

I needed to add the files to CMakeLists in order for the files to be included in the Jenkins test build to pass tests. I figured that making the setup.py build work as well would be desired, in case we switch Jenkins to use it rather than cmake. But I can take that out if desired.

The files are not generated at build time, they live in the codebase and engineers would generate them at the time they write their tests. They are copied during build time, and I guess subsequently copied during test time.

I'll remove the changes from setup.py then.

Are repo files available to Jenkins during the time of testing? If so, I can try to figure out to directly read from the repo location if so. Otherwise, I'd need to keep the CMakeLists.txt changes to allow Jenkins to continue working.

In that case, yes, all source controlled files are available during tests, at the source code locations.

By the way, how big are these files going to be? If someone checks in a large binary file, that binary size is permanent in the git repository, and constitutes a tax on everyone who every git clones the repo at any point in the future from there. Let's make it hard for people to accidentally do this.

@ezyang for the model and input/output data, they are small, just like the test data we committed to onnx repo. They should be fine. The goal to detect the incompatible changes on gradient ops to prevent sev (live the broadcasting one).

These are the sizes of the binary files as of now.

-rw-r--r--. 1 ansha users  67 Aug 28 11:36 gradient_0.pb
-rw-r--r--. 1 ansha users 390 Aug 28 11:37 inputs.npz
-rw-r--r--. 1 ansha users  43 Aug 28 11:37 operator_0.pb
-rw-r--r--. 1 ansha users 234 Aug 28 11:37 outputs.npz

I can add a test in the next diff to warn against binaries that are too large and warn if we surpass some limit on the entire directory of caffe2/python/serialized_test/data.

I've been looking through the test output files and discussing with @ezyang to figure out what we can do. Seems like currently the source code files live in /var/lib/jenkins/workspace/[...]. We can change CI to cd into WORKSPACE instead, before running these operator tests.

Do we really believe that checked in tests are a good idea? I'm pretty sure that having more rigorous test harness will provide much better coverage, and have a benefit of not checking in a new file after every single operator change. Here's how I see it:

Operator code is generally quite self-contained, so it's unlikely that a change in one part of the system will affect a random operator on the other end. Unless it's a breakage so bad that most operator tests will be red anyway. On the other hand, if someone changes the operator kernels, the tiny numerical differences might change e.g. the few least-significant bits, raising a failure when compared to the expected test. Since this was a PR that updates an op, I believe the author will be more than happy to check in a new set of expected outputs, meaning that those values don't mean much. Finally, you can't test all ops this way, because if you use an external library, just updating it to a new version (or having a different cuDNN version installed) might result in those tiny differences, flaring up the whole test suite for no good reason.

We actually took a similar approach for testing the PyTorch JIT some time ago (we serialize IR as text, not binary tensor values), but it has many scalability problems, and is notoriously hard to review. We already had many situations where the test had a good expect file checked in, only to have it overwritten with a non-working one in a later PR (and we do pretty careful code review). After the release, we'll probably try to move away from comparing to expected values, and you might want to consider that too.