Skip to content

[PoC] move metadata computation from prototype features into kernels #6646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Sep 29, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Sep 26, 2022

As discussed offline. Moving the computation to the kernels has several upsides:

  1. We would have the computation in one place rather than scattered on the kernel as well as on the feature. In some cases that might also be a performance improvement since we don't need to recompute intermediate values.
  2. Users that want to use the prototype kernels directly currently don't have access to the metadata we compute and thus have to do it themselves. This somewhat defeats the idea of providing kernels for all the transforms since we only provide part of the computation.

The only downside that I see is that now some kernels don't have a more complex output. Since there is no hard rule that the kernels need to return a single tensor, this should not effect anything but our test suite. The test suite currently at some places depends on a single tensor return, but this is trivial to change. I have not touched the tests yet, so they are currently failing.

@vfdev-5 also suggested that the kernels could have a return_metadata: bool = False flag. Using this, the users by default will always only get a tensor back, while they have the option to get the metadata as well. I'm usually against changing the return type through an input flag, but the consistent return type for all kernels is also appealing. However, if we want to go for that, we need to check if JIT is happy as well. The annotation, e.g. Union[torch.Tensor, Tuple[torch.Tensor, Tuple[int, int]]] for the bounding box kernels from this PR, should not be an issue, but I'm not sure if JIT can work with the output, i.e.

output1 = kernel(...)
output2, metadata = kernel(..., return_metadata=True)

If it can't, I'm strongly against this variant, because we then need more workarounds or the like that make the API more complex than a few kernels that return more than just a tensor.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm in favour of this approach. I think the return_metadata=True idea might bring some further speed gains but I don't believe it's JIT compatible (we do a conditional return on Object Detection models an it's exceptionally hacky to make it JIT-scriptable).

I think before merging this, we need to fix all the JIT-scriptability related PRs we have to ensure nothing breaks it.

@pmeier Are these the only kernels that need to return meta-data or it's just an example and there are more to be changed?

Copy link
Collaborator Author

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the only kernels that need to return meta-data or it's just an example and there are more to be changed?

No, these are all kernels that need to be changed.

@pmeier pmeier marked this pull request as ready for review September 27, 2022 10:04
Conflicts:
	torchvision/prototype/features/_bounding_box.py
	torchvision/prototype/transforms/functional/_geometry.py
@pmeier pmeier requested a review from datumbox September 29, 2022 11:13
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with a few minor comments.

@vfdev-5 could you please also have a look?

@pmeier pmeier merged commit ae83c9f into pytorch:main Sep 29, 2022
@pmeier pmeier deleted the kernel-metadata branch September 29, 2022 17:35
facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2022
… kernels (#6646)

Summary:
* move metadata computation from prototype features into kernels

* fix tests

* fix no_inplace test

* mypy

* add perf TODO

Reviewed By: datumbox

Differential Revision: D40138729

fbshipit-source-id: 871cafbb38946b4778abd0febd9ba5a2b6d1ef92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants