Skip to content
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

Add documentation for torch.overrides submodule. #48170

Closed
wants to merge 4 commits into from

Conversation

hameerabbasi
Copy link
Collaborator

Fixes #48087

@dr-ci
Copy link

dr-ci bot commented Nov 18, 2020

💊 CI failures summary and remediations

As of commit 32eed06 (more details on the Dr. CI page):


  • 2/2 failures possibly* introduced in this PR
    • 2/2 non-CircleCI failure(s)

Extra GitHub checks: 1 failed


ci.pytorch.org: 1 failed


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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 21 times.

@hameerabbasi
Copy link
Collaborator Author

@ezyang Now that these things are being documented, it's our last chance to make anything private. So let me know if you'd make anything else private.

torch/overrides.py Outdated Show resolved Hide resolved
@ezyang
Copy link
Contributor

ezyang commented Nov 18, 2020

I looked over all the funcs, and I agree with your assessment of what should be public and private.

@@ -605,14 +607,14 @@ provides a developer-facing API for ensuring full support for
changes without warning in the future.

First, to get a listing of all overridable functions, use
``torch.overrides.get_overridable_functions``. This returns a dictionary whose
``torch.overrides._get_overridable_functions``. This returns a dictionary whose
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh... I forgot that you refer to this explicitly in the docs. Referencing a private function here is kind of weird. Perhaps that means this particular function should be kept public. _get_testing_overrides and _get_ignored_functions are the ones that definitely should not be public.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Version 1.7 is the only version where these were public. Before that, the submodule was private. v1.7.1 is the only version where the docs might show it as public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, it doesn't have to do with the historical context is. What I mean is, it's weird to mark a function with a leading underscore, and then immediately turn around and in the docs say, "Hey, you should call this private function to find out what functions to implement it with!" It's like saying, "This thing, which you're not supposed to use, is so useful that we are going to tell you about it in the public documentation hint hint wink wink"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make these public.

@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #48170 (0dbf0d7) into master (49f0e5d) will decrease coverage by 0.00%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master   #48170      +/-   ##
==========================================
- Coverage   81.31%   81.31%   -0.01%     
==========================================
  Files        1839     1839              
  Lines      198692   198703      +11     
==========================================
+ Hits       161561   161569       +8     
- Misses      37131    37134       +3     

@hameerabbasi hameerabbasi marked this pull request as ready for review November 24, 2020 14:51
@hameerabbasi
Copy link
Collaborator Author

hameerabbasi commented Nov 25, 2020

The CI failures are all unrelated.

  • For the flake8 failure, see Pin flake8 version in CI #24188 (comment).
  • CircleCI macOS build had an infrastructure issue.
  • The failing test for ROCm is test_streaming_backward_sync_graph_root (__main__.TestCuda) (Tensors failed to compare as equal!), which has nothing to do with this PR either.

@hameerabbasi hameerabbasi requested review from ezyang and removed request for mattip November 25, 2020 10:17
@ngimel ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 26, 2020
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 4e15877.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged module: docs Related to our documentation, both in docs/ and docblocks module: __torch_function__ open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation for the torch.overrides submodule.
5 participants