Skip to content

Improve error message on invalid values to Distribution methods #61056

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
Closed
wants to merge 5 commits into from
Closed

Improve error message on invalid values to Distribution methods #61056

wants to merge 5 commits into from

Conversation

fritzo
Copy link
Collaborator

@fritzo fritzo commented Jun 30, 2021

Fixes #18133

@fritzo fritzo added the module: distributions Related to torch.distributions label Jun 30, 2021
@fritzo fritzo requested a review from neerajprad June 30, 2021 18:10
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 30, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit 50d17e2 (more details on the Dr. CI page and at hud.pytorch.org/pr/61056):

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

Preview docs built from this PR

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.

@fritzo fritzo mentioned this pull request Jun 30, 2021
Closed
@fritzo fritzo force-pushed the informative-dist-support-error branch from a042b18 to 2a821fc Compare June 30, 2021 18:19
fritzo
fritzo commented Jun 30, 2021
View reviewed changes
neerajprad
neerajprad reviewed Jun 30, 2021
View reviewed changes
Copy link
Contributor

@neerajprad neerajprad left a comment

Choose a reason for hiding this comment

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

Thank you for improving the error message! Could you remove the change to the CI workflow? I think we can handle that in a separate PR if needed and get this merged sooner.

neerajprad
neerajprad reviewed Jun 30, 2021
View reviewed changes
@fritzo fritzo force-pushed the informative-dist-support-error branch from a872394 to 5276424 Compare June 30, 2021 23:53
@fritzo
Copy link
Collaborator Author

fritzo commented Jul 1, 2021

I believe remaining test errors are unrelated.

@facebook-github-bot
Copy link
Contributor

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

@fritzo
Copy link
Collaborator Author

fritzo commented Jul 1, 2021

Thanks for reviewing @neerajprad!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@neerajprad merged this pull request in 81e36d0.

@eellison
Copy link
Contributor

eellison commented Jul 7, 2021

cc @neerajprad looks like this broke lint

@neerajprad
Copy link
Contributor

cc @neerajprad looks like this broke lint

@eellison: As discussed in another thread, this doesn't touch any files that are currently being flagged by mypy. I think the change is due to changes in the linter settings itself introduced in #61119.

This was referenced Nov 3, 2021
Closed
Closed
facebook-github-bot pushed a commit that referenced this pull request Nov 3, 2021
@facebook-github-bot
Remove duplicate check in distributions arg validation (#67741)
201f7d3 
Summary:
Partial fix for #66800. (Duplicate of #67725 against pytorch/pytorch so as to trigger TorchBench)

#61056 added a more verbose error message for distributions failing argument validation. However, it did not replace the earlier error check as was originally intended and was flagged by xuzhao9 as being the potential cause of a perf regression in `test_eval[soft_actor_critic-cuda-eager]`.

xuzhao9: Is there a way for me to check if this resolves the perf issue you mentioned?

cc VitalyFedyunin ngimel

Note that existing tests already check for the error message and should verify that the removed lines are redundant.

RUN_TORCHBENCH: soft_actor_critic

Pull Request resolved: #67741

Reviewed By: neerajprad

Differential Revision: D32135675

Pulled By: xuzhao9

fbshipit-source-id: 37dfd3ff53b95017c763371979ab3a2c302a72b9
@github-actions github-actions bot deleted the informative-dist-support-error branch February 12, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@neerajprad neerajprad neerajprad approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla signed Merged module: distributions Related to torch.distributions open source
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Descriptive error message for distribution constraint violation
5 participants
@fritzo @facebook-github-bot @eellison @neerajprad @pytorchbot