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

make onnx expect tests resiliant to producer_version changes #39002

Closed
wants to merge 2 commits into from

Conversation

@mattip
Copy link
Contributor

mattip commented May 26, 2020

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.

@ezyang
ezyang approved these changes May 26, 2020
@ezyang
Copy link
Contributor

ezyang commented May 26, 2020

TBH, it would be better to replace it with some more meaningful signifier, but I won't block the PR on that.

Copy link
Contributor

facebook-github-bot left a comment

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 27, 2020

@ezyang merged this pull request in 2e6ee85.

robieta added a commit that referenced this pull request Jun 4, 2020
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
facebook-github-bot added a commit that referenced this pull request Jul 28, 2020
Summary:
xref gh-39002 which handled the reading but not the writing of the onnx expect files, and the last comment in that PR which points out `XXX` was suboptimal.
xref [this comment](#37091 (comment)) which pointed out the problem.

This PR:
- replaces `XXX` with `CURRENT_VERSION` in the stored files
- ensures that updating the results with the `--accept` flag will maintain the change

Pull Request resolved: #41910

Reviewed By: pbelevich

Differential Revision: D22758671

Pulled By: ezyang

fbshipit-source-id: 47c345c66740edfc8f0fb9ff358047a41e19b554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.