KEMBAR78
remove pytest.UsageError by pmeier · Pull Request #58916 · pytorch/pytorch · GitHub
Skip to content

Conversation

@pmeier
Copy link
Collaborator

@pmeier pmeier commented May 25, 2021

Stack from ghstack:

Using pytest.UsageError in case pytest is available adds almost
nothing as observed in
#53820 (comment), but
makes it harder to maintain: due to the conditional import, mypy is
not able to handle UsageError in a type annotation.

Differential Revision: D29259409

Using `pytest.UsageError` in case `pytest` is available adds almost
nothing as observed in
#53820 (comment), but
makes it harder to maintain: due to the conditional import, `mypy` is
not able to handle `UsageError` in a type annotation.

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 25, 2021

💊 CI failures summary and remediations

As of commit 6aeb57b (more details on the Dr. CI page and at hud.pytorch.org/pr/58916):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

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

Click here to manually regenerate this comment.

Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Cool simplification

Using `pytest.UsageError` in case `pytest` is available adds almost
nothing as observed in
#53820 (comment), but
makes it harder to maintain: due to the conditional import, `mypy` is
not able to handle `UsageError` in a type annotation.

[ghstack-poisoned]
Using `pytest.UsageError` in case `pytest` is available adds almost
nothing as observed in
#53820 (comment), but
makes it harder to maintain: due to the conditional import, `mypy` is
not able to handle `UsageError` in a type annotation.

[ghstack-poisoned]
Using `pytest.UsageError` in case `pytest` is available adds almost
nothing as observed in
#53820 (comment), but
makes it harder to maintain: due to the conditional import, `mypy` is
not able to handle `UsageError` in a type annotation.

[ghstack-poisoned]
@mruberry
Copy link
Collaborator

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in cf789b9.

@NicolasHug
Copy link
Member

NicolasHug commented Jun 25, 2021

Oops, this broke torchvision's tests :)

We were relying on pytest.UsageError: https://github.com/pytorch/vision/pull/4121/files

I understand and agree with the removal of pytest.UsageError.

The docs clearly mention that a UsageError error is raised, but this exception is private. Perhaps we could make it public?

Altough, TBF, I would personally prefer if this was just a ValueError, which seems to qualify for all of the current scenarios:

        UsageError: If a :class:`torch.Tensor` can't be constructed from an array-or-scalar-like.
        UsageError: If any tensor is quantized or sparse. This is a temporary restriction and will be relaxed in the
            future.
        UsageError: If only :attr:`rtol` or :attr:`atol` is specified.

and will avoid users (us) to import a custom exception from torch.testing (which also can add extra confusion as it's very similar to pytest.UsageError)

@facebook-github-bot facebook-github-bot deleted the gh/pmeier/17/head branch June 25, 2021 14:19
pmeier added a commit that referenced this pull request Jul 1, 2021
pmeier added a commit that referenced this pull request Jul 1, 2021
pmeier added a commit that referenced this pull request Jul 6, 2021
pmeier added a commit that referenced this pull request Jul 6, 2021
pmeier added a commit that referenced this pull request Jul 8, 2021
pmeier added a commit that referenced this pull request Jul 8, 2021
pmeier added a commit that referenced this pull request Jul 9, 2021
pmeier added a commit that referenced this pull request Jul 9, 2021
facebook-github-bot pushed a commit that referenced this pull request Jul 9, 2021
Summary:
Pull Request resolved: #61031

See #58916 (comment).

Test Plan: Imported from OSS

Reviewed By: iramazanli

Differential Revision: D29626810

Pulled By: mruberry

fbshipit-source-id: 25ddf26815f9ef82b8234d7dac811a6a13a53c54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged module: testing Issues related to the torch.testing module (not tests) open source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants