Skip to content

split up community documentation, add build/test document #38629

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 3 commits into from
Closed

split up community documentation, add build/test document #38629

wants to merge 3 commits into from

Conversation

@mattip
Copy link
Contributor

@mattip mattip commented May 17, 2020 •
edited
Loading

As part of the fallout from gh-36797, this PR

  • replaces the producer_version: "1.6" in expect tests with `producer_version: "XXX"
  • adapts testing/internal/common_utils.py with a regex to change the onnx producer_version so tests still pass
  • refactor the "Community" documentation into smaller chunks
  • add a testing.rst to start to explain how to build and test pytorch locally
    • still a WIP, but maybe usable as-is?
    • document the onnx test EXPECTTEST_ACCEPT=1 option

Edit: split out the first two items to gh-39002

mattip
mattip commented May 17, 2020
View reviewed changes
docs/source/index.rst Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

along the way, fix this documentation problem. rpc/index.rst does not exist

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @mattip, that file was actually removed by this PR #37419. If you don't plan to add it back, we can create a PR to restore it before v1.6 branch cut, otherwise the RPC package is excluded from the index page. Please let us know if you have a different plan. Thanks!

cc @rohan-varma @jlin27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rpc.rst document still exists, and has links to the rpc/*.rst documents.

@dr-ci
Copy link

dr-ci bot commented May 17, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 493dc24 (more details on the Dr. CI page):

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

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 8 times.

@mattip
Copy link
Contributor Author

mattip commented May 18, 2020

The test failure is unrelated:

Running distributed/test_nccl ... [2020-05-17 18:16:46.668091]
test_all_gather (__main__.TestNCCL) ... ok
test_all_reduce (__main__.TestNCCL) ... ok
test_broadcast (__main__.TestNCCL) ... FAIL
test_reduce (__main__.TestNCCL) ... FAIL
test_reduce_scatter (__main__.TestNCCL) ... FAIL
test_unique_id (__main__.TestNCCL) ... ok

@mattip mattip mentioned this pull request May 20, 2020
Closed
kurtamohler
kurtamohler reviewed May 20, 2020
View reviewed changes
@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 21, 2020
@mattip
Copy link
Contributor Author

mattip commented May 22, 2020

All the top-level *.md pages in the repo should be .. include:: -ed in the community documentation. For instance CONTRIBUTING.md mentions compile-time defines that are not mentioned in the RST documentaiton.

@rgommers
Copy link
Collaborator

@mattip the changes here look good, but there's nothing in this PR related to producer_version and ONNX tests?

mattip
mattip commented May 25, 2020
View reviewed changes
Copy link
Contributor Author

@mattip mattip left a comment

Choose a reason for hiding this comment

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

True, this was to relate to "document how to regenerate the ONNX expect results". A bit much for 4 lines, but I couldn't resist.

docs/source/community/testing.rst Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the documentation reformatting was to get to this: how to regenerate ONNX test results

@rgommers
Copy link
Collaborator

You should probably edit the PR description here, and make the actual ONNX test changes in a separate PR.

@mattip mattip mentioned this pull request May 26, 2020
Closed
facebook-github-bot pushed a commit that referenced this pull request May 26, 2020
@mattip @facebook-github-bot
make onnx expect tests resiliant to producer_version changes (#39002)
2e6ee85 
Summary:
closes gh-32561 closes gh-38545. As part of the fallout from gh-36797, this PR
- replaces the producer_version: "1.6" in onnx expect tests with `producer_version: "XXX"
- adapts `testing/_internal/common_utils.py` with a regex to change the onnx producer_version so tests still pass

The consistency of the torch version and the onnx `producer_version` is tested in gh-36797, so there is no reason to test it again in the expect tests.

xref gh-38629 which documented how to run the onnx tests and at the same time refactored the Community documentation.
Pull Request resolved: #39002

Differential Revision: D21723062

Pulled By: ezyang

fbshipit-source-id: 1bd6a8ed37d5383e69d017226dc09c0645a69aff
@mattip
Copy link
Contributor Author

mattip commented Jun 9, 2020

ping @jlin27

mrshenli
mrshenli requested changes Jun 15, 2020
View reviewed changes
docs/source/index.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @mattip, that file was actually removed by this PR #37419. If you don't plan to add it back, we can create a PR to restore it before v1.6 branch cut, otherwise the RPC package is excluded from the index page. Please let us know if you have a different plan. Thanks!

cc @rohan-varma @jlin27

@rgommers rgommers requested a review from jlin27 August 7, 2020 22:09
@mattip
Copy link
Contributor Author

mattip commented Sep 8, 2020

Rebased to clear conflict caused by http -> https transition.

@mrshenli mrshenli self-requested a review September 8, 2020 16:01
@pytorch pytorch deleted a comment from codecov bot Sep 18, 2020
@facebook-github-bot
Copy link
Contributor

Hi @mattip!

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but we do not have a signature on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@mattip
Copy link
Contributor Author

mattip commented Feb 18, 2021

Closing, too low priority to continue.

@mattip mattip closed this Feb 18, 2021
@mattip mattip mentioned this pull request Apr 22, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kurtamohler kurtamohler kurtamohler left review comments

@jlin27 jlin27 Awaiting requested review from jlin27

@mrshenli mrshenli Awaiting requested review from mrshenli

Assignees

No one assigned

Labels

cla signed open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mattip @rgommers @facebook-github-bot @kurtamohler @mrshenli @ailzhang @pytorchbot