Make binary_cross_entropy_with_logits composite compliant

[drisspg]

Fixes one of the tests related to this issue #75680
Removes the skip for the composite compliance test_backward and updates implementation of binary_cross_entropy_with_logits so that the correct inplace and out of place operators are used depending on tensor subclasses being present

[facebook-github-bot]

🔗 Helpful links

• 🧪  See artifacts and rendered test results at hud.pytorch.org/pr/75929
• 📄  Preview Python docs built from this PR
• 📄  Preview C++ docs built from this PR
• ❓Need help or want to give feedback on the CI? Visit our office hours
• ↩️  [fb-only] Re-run with SSH instructions

💊 CI failures summary and remediations

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

Expand to see more

---

• 1/1 failures introduced in this PR

---

🕵️‍♀️ 1 failure not recognized by patterns:

The following CI failures may be due to changes from the PR

Job	Step	Action
pull / linux-bionic-rocm5.0-py3.7 / test (default, 2, 2, linux.rocm.gpu)	Unknown	🔁 rerun

---

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[albanD]

It is a bit worrying that the test don't catch the fact that weights are ignored here :(

[ngimel]

binary_cross_entropy_with_logits is not a composit implicit autograd operation, so why is it failing the test? Also, it doesn't look like it's calling resize_, all it does is calling some inplace ops on an intermediate tensor - is it not allowed for composite ops?

[albanD]

binary_cross_entropy_with_logits is not a composit implicit autograd operation, so why is it failing the test?

That is a very good question! cc @zou3519 ?

all it does is calling some inplace ops on an intermediate tensor

It depends on which ones. You can only do inplace with Tensors that are guaranteed to be of the same "type".
For example (input + 1).mul_(target) would fail if input is a plain Tensor (and thus input + 1 will also be a plain Tensor) while target is a subclass.

[zou3519]

binary_cross_entropy_with_logits is not a composit implicit autograd operation, so why is it failing the test?

That is a very good question! cc @zou3519 ?

Composite compliance applies to CompositeImplicitAutograd pytorch operations OR a function (in python or c++) that calls PyTorch operations (a "composite").

binary_cross_entropy_with_logits's backward formula is not a PyTorch operator -- it's a C++ function that calls Pytorch operations. The backward formula is the thing that is not composite compliant.

[drisspg]

binary_cross_entropy_with_logits is not a composit implicit autograd operation, so why is it failing the test?

That is a very good question! cc @zou3519 ?

Composite compliance applies to CompositeImplicitAutograd pytorch operations OR a function (in python or c++) that calls PyTorch operations (a "composite").

binary_cross_entropy_with_logits's backward formula is not a PyTorch operator -- it's a C++ function that calls Pytorch operations. The backward formula is the thing that is not composite compliant.

Should I change forward formula back to what it was with mostly inplace operations that aren't switched on subclass tensors - and keep the backward update?

[ngimel]

@zou3519 So the fixes need to exist only in binary_cross_entropy_with_logits_backward, not binary_cross_entropy_with_logits, at least for the purposes of the test? I understand Alban's concern that even for explicit autograd op subclassing won't be propagated with inplace ops.
Should the note about inplace ops and sublassing be added somewhere? Because currently README only warns about resizing in CompositeImplicitAutograd ops.

[drisspg]

It is a bit worrying that the test don't catch the fact that weights are ignored here :(

This got me thinking and found I wrote a terrible sampling function which I think I corrected allowing me to remove a few more skips

[zou3519]

@zou3519 So the fixes need to exist only in binary_cross_entropy_with_logits_backward, not binary_cross_entropy_with_logits, at least for the purposes of the test? I understand Alban's concern that even for explicit autograd op subclassing won't be propagated with inplace ops.

Yes, sorry for the delayed reply.

Should the note about inplace ops and sublassing be added somewhere? Because currently README only warns about resizing in CompositeImplicitAutograd ops.

Yes... I thought it was there but apparently it's not

[zou3519]

Fix for composite compliance LGTM. Since we're modifying the OpInfo, I have some questions about the semantics of the operator

[drisspg]

Just Rebased onto master and this skip:

DecorateInfo(
                unittest.expectedFailure,
                'TestCompositeCompliance',
                'test_forward_ad'),

Is now failing for performing inplace ops on tensor subclass. I am going to add back in this skip and not update the forward pass because see above convseration

[zou3519]

Thank you!

[drisspg]

@pytorchbot merge this please

[github-actions]

Hey @drisspg.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.