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 new .. betastatus:: directive to document Beta APIs #6115

Merged
merged 6 commits into from Jun 14, 2022

Conversation

NicolasHug
Copy link
Member

This PR adds a new .. betastatus :: <api_name> directive to show a warning in the docs for API surfaces that are still Beta. It can be used on entire classes, function, or specific parameter.

It renders like a normal .. warning:: (this can be changed):

image

@mthrok Following up on our internal discussion Would you mind sharing your thoughts on this? It'd be great if we could align regarding the documentation of Beta APIs. We don't need to agree on the underlying implementation, but letting users know about this in the docs is what matters. Thanks!

@NicolasHug NicolasHug marked this pull request as draft May 31, 2022 11:25
@NicolasHug
Copy link
Member Author

Converting to draft to avoid merging before @mthrok can give his thoughts, but this is good to review and stable on my side.

@datumbox
Copy link
Contributor

@NicolasHug Nice, I think that can handle both whole methods and specific parameters.

@fmassa I wonder if you have a list of APIs that are currently on beta or you could provide a few references, so that we can document the entire TorchVision.

@mthrok
Copy link
Contributor

mthrok commented May 31, 2022

I think keeping the status value as argument is more flexible, such as ..api_status:: [beta, stable, prototype].
With the current approach, stable API can be only recognized by the lack of the directive.
I think this is error prune, considering the possibility where one adds a new feature but forgets to add the badge that says beta. Forgetting to add the directive automatically promotes the API to stable on the doc.

This would also allow to communicate the context of status change after moving it out of beta like This API has been promoted to stable in release "<>"..

@NicolasHug
Copy link
Member Author

Thanks for your input @mthrok .

I originally had it like you mentioned ..api_status:: [beta, stable, prototype], but decided against it because:

  • we don't actually document prototype stuff, although this might change
  • A reasonable assumption is that if something isn't beta, then it's stable.

I don't feel strongly about this though. I'm open to using something like this, but then I'd like your thoughts on the following points:

  • does that mean we would write api_status:: stable on all existing stable classes / functions ? If we say that a function is stable but one of its parameter is Beta (like the decode_jpeg() above), could this be potentially confusing?
  • What should be the rendering of .. api_status:: stable ? Should be it like a .. note:: ? Or a badge? Something else?

This would also allow to communicate the context of status change after moving it out of beta like This API has been promoted to stable in release "<>"..

I agree this is good to document. Typically this could be done via the builtin .. versionchanged:: directive (but I don't remember if it renders properly with our theme).

@datumbox
Copy link
Contributor

+1 on @mthrok's idea to include information on the docs about when a specific API became available.

Concerning marking everything versus marking only beta features, I agree with @NicolasHug that marking every module, class, method (or parameter) with a "stable" badge can be noisy. I think @mthrok does have a point about the fact that explicitly marking something is less error prone but I'm not sure what would be the policy to avoid putting badges all over the place. No strong opinions from me either, I think it's always OK to test something and get the input from the community rather than wait for the perfect solution. What's next to unblock this work?

@NicolasHug NicolasHug marked this pull request as ready for review June 14, 2022 09:35
@NicolasHug
Copy link
Member Author

NicolasHug commented Jun 14, 2022

As discussed offline with @mthrok there's no blocker on his side. Let's move forward with this solution, we can of course revisit at any point in time regarding the underlying implementation.

I added the directive to all detection model builders and individual doc pages as well. I'll check with @fmassa later today if we missed anything, but this is good to go on my side.
EDIT: also added semantic segmentation and video models

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.

LGTM, thanks!

@NicolasHug NicolasHug merged commit 0e688ce into pytorch:main Jun 14, 2022
NicolasHug added a commit to NicolasHug/vision that referenced this pull request Jun 14, 2022
)

* Add new .. betastatus:: directive to document Beta APIs

* Also add it for the fine-grained video API

* Add directive for all builders and pages of Detection module

* Also segmentation and video models
NicolasHug added a commit that referenced this pull request Jun 14, 2022
…6165)

* Add new .. betastatus:: directive to document Beta APIs

* Also add it for the fine-grained video API

* Add directive for all builders and pages of Detection module

* Also segmentation and video models
NicolasHug added a commit to NicolasHug/vision that referenced this pull request Jun 16, 2022
…ytorch#6115)

Summary:
* Add new .. betastatus:: directive to document Beta APIs

* Also add it for the fine-grained video API

* Add directive for all builders and pages of Detection module

* Also segmentation and video models

Differential Revision: D37212650

fbshipit-source-id: 328eb282b660c44972aec4134acff31c9310ace2
facebook-github-bot pushed a commit that referenced this pull request Jun 17, 2022
…6115)

Summary:
* Add new .. betastatus:: directive to document Beta APIs

* Also add it for the fine-grained video API

* Add directive for all builders and pages of Detection module

* Also segmentation and video models

Reviewed By: datumbox

Differential Revision: D37212650

fbshipit-source-id: 05bfde6fcb9d07238777daeac3814f4c94ddf718
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.

None yet

4 participants