The docs for __torch_function__ will also need to be updated, see pytorch/docs/source/notes/extending.rst.

Ping @cpuhrsch, who may have some insight on how annoying the break in the API for __torch_function__ will be, at least for NestedTensor.

I'm not sure if this would introduce runtime cost but it may be worth either having effectively a try/except in C++ as well as in the Python handle_torch_function implementation to accomodate the old __torch_function__ API, or to generate a nicer error message, in case there are any comsumers of this functionality already with PyTorch 1.4.

The docs for __torch_function__ will also need to be updated, see pytorch/docs/source/notes/extending.rst.

Done.

@hameerabbasi adding benchmark results for this similar to how @ngoldbaum did this before would be good to do next, seems critical. There's docs on using py-spy for this

Yes, benchmarks please!

💊 CircleCI build failures summary and remediations

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

✅ None of the build failures appear to be your fault 💚

• 2/2 broken upstream at merge base d9b97a4 on Mar 17 from 8:00am to 6:10pm (9 commits; d9b97a4 - 3e68d0c)

  Please rebase on the viable/strict branch (expand for instructions)

  If your commit is newer than viable/strict, you can try basing on an older, stable commit:

  git fetch https://github.com/pytorch/pytorch viable/strict
  git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)
  

  If your commit is older than viable/strict:

  git fetch https://github.com/pytorch/pytorch viable/strict
  git rebase FETCH_HEAD
  

  Check out the recency history of this "viable master" tracking branch.

🚧 1 upstream failure:

These were probably caused by upstream breakages:

• pytorch_windows_vs2019_py36_cuda10.1_build on Mar 17 from 8:00am to 6:10pm (9 commits; d9b97a4 - 3e68d0c)

Benchmarks

This branch:

$ python -m pt.add_test --test_name add_M1_N1_K1_cpu --omp_num_threads 1 --mkl_num_threads 1
# ----------------------------------------
# PyTorch/Caffe2 Operator Micro-benchmarks
# ----------------------------------------
# Tag : None

# Benchmarking PyTorch: add
# Mode: Eager
# Name: add_M1_N1_K1_cpu
# Input: M: 1, N: 1, K: 1, device: cpu
Forward Execution Time (us) : 2.036

The commit before it diverges from master:

$ python -m pt.add_test --test_name add_M1_N1_K1_cpu --omp_num_threads 1 --mkl_num_threads 1
# ----------------------------------------
# PyTorch/Caffe2 Operator Micro-benchmarks
# ----------------------------------------
# Tag : None

# Benchmarking PyTorch: add
# Mode: Eager
# Name: add_M1_N1_K1_cpu
# Input: M: 1, N: 1, K: 1, device: cpu
Forward Execution Time (us) : 2.176

Although from running the benchmark a couple of times, everything seems to be well within noise.

@hameerabbasi we went through this a bunch of times before, asv is pretty bad at measuring anything with the required accuracy here. Please talk to @ngoldbaum for how to best do this, and/or look at his __torch_function__ PRs.

@rgommers those are the benchmarks I used in those PRs.

@rgommers This is PyTorch's built-in test suite, not ASV. Also, I highly doubt adding an extra argument is going to affect performance in a measurable way. 😉

Hmm, I found the py-spy story much more convincing. The report here is a gain of 140 ns. Given that that's close to 10% of the total call overhead, I think it's important to not wave away?

Here are the two py-spy SVGs, one for each case.

Like I said, this is due to noise. I could re-run tests, but I ran each test a couple of times but reported the first run of each. The subsequent runs show a large variation, larger than is reported here (+-200 ns IIRC).

So, it's possible due to pure noise that my addition had a reported performance better than master, even if that isn't based in reality.

Also, we're looking at the case of a one-element add, where the computation is basically nothing, and the whole cost is the machinery.

The diff looks basically fine but I'd love to see the benchmarks (EDIT: oops forgot to refresh)

@hameerabbasi Some questions about benchmark hygiene:

• What compile options did you use to compile PyTorch?
• Can you test on functions with more arguments? We expect the cost of allocating the types python tuple to scale linearly with the number of tensor-like arguments. If it remains invisible even for a function with a lot of tensor arguments, there's something wrong with the benchmark.

not sure that's the right flamegraph

You're right, I updated it.

Would a new script with procedure in its docstring, and linking to that from torch/_overrides.py, be the right place for that?

That sounds reasonable to me. It will be difficult to get people to find out about the script but if we at least know about it ourselves that will be good enough for now.

It will be difficult to get people to find out about the script

We can just point to it from the critical parts of the __torch_function__ mechanism. One only needs these benchmarks when touching those parts, so it shouldn't be too bad.

Should I leave that to this PR or a follow-up?

Separate PR seems better; that PR could be merged straight away, while I think this one we may want to consider together with the make_subclass one and the to-be-submitted methods overrides one.

can you please merge with master

Just FYI #34240 is still in progress of landing.

Thanks Christian. We'll hold this PR for that one, and then remerge.

Benchmark PR at #34645.

#34240 was merged

Re-merged.

Looks like internal tests are failing.

@hameerabbasi I can't seem to push to your branch (did you fail to allow maintainers to update the branch). Could you please merge with master?

did you fail to allow maintainers to update the branch

That might've been unchecked by default, and I can't find the checkbox where it usually is on the side of the PR.

@cpuhrsch heads up this will break you

@ezyang merged this pull request in 6b701de.