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
Collaborator

@mattip 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
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 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 is landing 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 2e6ee85.

facebook-github-bot pushed 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
Development

Successfully merging this pull request may close these issues.

ENH: refactor onnx expecttests PyTorch 1.4 ONNX Producer Version Mismatch
5 participants