Skip to content

reverting some recently introduced exceptions #5659

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Mar 29, 2022
Merged

reverting some recently introduced exceptions #5659

merged 27 commits into from
Mar 29, 2022

Conversation

jdsgomes
Copy link
Contributor

@jdsgomes jdsgomes commented Mar 22, 2022 •
edited
Loading

Removing exceptions and asserts from the forward pass in order to enable FX traceability. Instead reverting to torch._assert which is FX traceable

@jdsgomes jdsgomes requested a review from datumbox March 22, 2022 17:19
@facebook-github-bot
Copy link

facebook-github-bot commented Mar 22, 2022 •
edited
Loading

💊 CI failures summary and remediations

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

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

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.

datumbox
datumbox reviewed Mar 22, 2022
View reviewed changes
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.

Thanks @jdsgomes, just a few comments. Let me know your thoughts.

jdsgomes
jdsgomes commented Mar 24, 2022
View reviewed changes
datumbox
datumbox reviewed Mar 24, 2022
View reviewed changes
@datumbox
Copy link
Contributor

@jdsgomes The problem seems to be on the assertion message (type/tostring operator) of the test_detection_model tests. Have you tried changing the message to see if the issue gets resolved? There are also some other exceptions that are being triggered at test_boxes_shape. There might be a few more of them that might be handling.

Do you mind converting the PR to draft and ping us when you want a review?

@jdsgomes jdsgomes marked this pull request as draft March 25, 2022 15:43
@jdsgomes jdsgomes marked this pull request as ready for review March 28, 2022 17:13
datumbox
datumbox reviewed Mar 29, 2022
View reviewed changes
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.

Thanks @jdsgomes. I understand that's far from a perfect situation, I left one comment below and marked all places where it appears. Let me know your thoughts.

datumbox
datumbox approved these changes Mar 29, 2022
View reviewed changes
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.

Thanks for all the updates. Feel free to merge on green CI.

@jdsgomes jdsgomes merged commit 61f8266 into pytorch:main Mar 29, 2022
@github-actions
Copy link

Hey @jdsgomes!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request Apr 5, 2022
@jdsgomes @datumbox @facebook-github-bot
[fbsync] reverting some recently introduced exceptions (#5659)
ded6abe 
Summary:
* reverting some recently introduced exceptions

* Update torchvision/ops/poolers.py

* address PR comments

* replace one more assert with torch._assert:

* address PR comments

* make type checker happy

* Fix bug

* fix bug

* fix for wrong asserts

* attempt to make tests pass

* Fix test_ops tests

* Fix expected exception in tests

* fix typo

* fix tests and format

* fix flake8

* remove one last exception

* fix error

* remove unused immport

* replace fake returns by else

(Note: this ignores all push blocking failures!)

Reviewed By: YosuaMichael

Differential Revision: D35216770

fbshipit-source-id: 68d01d50d083070976a6cd7bd4a98cc270be9723

Co-authored-by: Vasilis Vryniotis <[email protected]>
Co-authored-by: Vasilis Vryniotis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datumbox datumbox datumbox approved these changes

Assignees
No one assigned
Labels
cla signed code quality topic: feature extraction
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdsgomes @facebook-github-bot @datumbox