Skip to content

expose some prototype transforms utils #6989

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 3 commits into from
Nov 30, 2022
Merged

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Nov 29, 2022

In order to make it easy to subclass the v2 transforms, we also need to expose some utilities for public usage. This PR adds a new module torchvision.prototype.transforms.utils for that. Everything else stays in _utils as before.

For completeness, we also discussed dynamically create this module. This can be achieved by adding the following to __init__.py

def _load_utils():
    import sys
    import types

    from torchvision.prototype.transforms import _utils

    utils = types.ModuleType(f"{__package__}.utils")
    utils.__package__ = __package__
    for attr in ["has_any", "has_all"]:  # TODO: expand this to everything we want to expose
        utils.__dict__[attr] = getattr(_utils, attr)
    sys.modules[utils.__name__] = utils

    return utils


utils = _load_utils()

del _load_utils

Now, one can do

>>> from torchvision.prototype.transforms import utils
>>> utils.has_any
<function has_any at 0x7f92516060e0>
>>> from torchvision.prototype.transforms.utils import has_any
>>> has_any
<function has_any at 0x7f92516060e0>

This would save us the split between the two modules, but has some issues as well:

  1. This is a fairly complex solution to an otherwise simple problem
  2. Due to the dynamic creation, imports such as from torchvision.prototype.transforms.utils import has_any work perfectly fine, but are not picked up by static analyzers like mypy or IDE's

cc @vfdev-5 @datumbox @bjuncek

return h, w


def _isinstance(obj: Any, types_or_checks: Tuple[Union[Type, Callable[[Any], bool]], ...]) -> bool:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only thing we need to deal with before we move forward.

Copy link
Contributor

@datumbox datumbox Nov 29, 2022

Choose a reason for hiding this comment

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

I had a look on the usage of _isinstance in our codebase and feel that it's useful enough to be a public utility. WDYT?

We obviously can't remove the underscore, so we need a new name. Perhaps check_type()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I would expose it as well. +1 for check_type. @vfdev-5 Do you have an opinion here?

@datumbox
Copy link
Contributor

When we merge this, we should update #6721 and #6534 to avoid breaking their code again.

@pmeier pmeier requested review from vfdev-5 and datumbox November 30, 2022 10:18
@pmeier pmeier marked this pull request as ready for review November 30, 2022 10:18
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.

@pmeier LGTM, thanks!

@vfdev-5 We are having issues with the flaky bbox resize tests. Could you please prioritise the fix?

@pmeier
Copy link
Collaborator Author

pmeier commented Nov 30, 2022

We are having issues with the flaky bbox resize tests. Could you please prioritise the fix?

See #6995.

@pmeier pmeier merged commit 4941c6b into pytorch:main Nov 30, 2022
@pmeier pmeier deleted the public-utils branch November 30, 2022 11:00
facebook-github-bot pushed a commit that referenced this pull request Dec 1, 2022
Summary:
* expose some prototype transforms utils

* rename _isinstance

Reviewed By: YosuaMichael

Differential Revision: D41648537

fbshipit-source-id: 3239b62cb3fc7a915208608d04775c3ed2a8281a
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