Skip to content

Model contrib guidelines #5315

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

Merged
merged 23 commits into from
Feb 4, 2022
Merged

Conversation

jdsgomes
Copy link
Contributor

New model contribution guidelines that aim to clarify the process for adding new model architectures or improved model weights.

Feedback welcome, as always, before it gets merged.

cc: @datumbox @xiaohu2015 @zhiqwang

@facebook-github-bot
Copy link

facebook-github-bot commented Jan 29, 2022

💊 CI failures summary and remediations

As of commit 8df96a9 (more details on the Dr. CI page):


None of the CI failures appear to be your fault 💚



🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed 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.

@jdsgomes
Copy link
Contributor Author

@xiaohu2015 @zhiqwang since you have been through this process recently your feedback would be very valuable :)

Copy link
Contributor

@zhiqwang zhiqwang left a comment

Choose a reason for hiding this comment

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

Hi @jdsgomes , This guidelines look very useful and informative to me. I think one thing needs to be supplemented is that the added model needs to provide at least torchscript export support (especially the torch.jit.script mode as torchvision is currently using).

- Highlight the important test metrics and how they compare to the original paper
- Highlight any design choices that deviate from the original paper/implementation and rationale for these choices

## New Model Architectures - Implementation Details
Copy link
Contributor

Choose a reason for hiding this comment

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

We've discussed with Joao the possibility of moving this section out of the guideline doc (perhaps to an issue or a wiki article?). This is because it's quite low-level and potentially subjective to regular change. I would like to hear the thoughts of the community on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for issue because it allows for further discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, moved to a new issue for now, if we decide to move it back later that's ok: #5319

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. Worth adding on the new issue an explicit note about JIT-scriptability as @zhiqwang said. Going forwards we also expect all models to have scriptable preprocessing transforms as part of the multi-weight API, though I would probably omit this until the API graduates from prototype.

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, though I wouldn't merge before the end of the week to see if there is more feedback.

Might be worth posting this link on previous issues, such as #2707 to let people know we work on a new contribution guideline.

@jdsgomes jdsgomes merged commit dad6e6a into pytorch:main Feb 4, 2022
@jdsgomes jdsgomes deleted the model-contrib-guidelines branch February 4, 2022 11:17
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

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 Feb 11, 2022
Summary:
* Adding multiweight support for shufflenetv2 prototype models

* Revert "Adding multiweight support for shufflenetv2 prototype models"

This reverts commit 31fadbe.

* Adding multiweight support for shufflenetv2 prototype models

* Revert "Adding multiweight support for shufflenetv2 prototype models"

This reverts commit 4e3d900.

* Remove module vs method name clash

* add model contribution guidelines

* update CONTRIBUTING_MODELS.md

* Fix formatting and typo

* fix in-document links

* Update CONTRIBUTING.md

* remove Implementation Details section

Reviewed By: NicolasHug

Differential Revision: D34140246

fbshipit-source-id: 0c96404ac238bb039b0e2d057c7db961a6ab0512

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants