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

Update diff bakend node tests for auto update doc #5604

Merged
merged 5 commits into from Sep 21, 2023

Conversation

jcwchen
Copy link
Member

@jcwchen jcwchen commented Sep 20, 2023

Description

  • Extend onnx/backend/test/cmd_tools.py with a new option: "diff", which will only update the backend node test data whose script has been updated.
  • Make auto_update_doc.yml CI utilize "--diff" and update the backend node test data accordingly.

Motivation and Context

#5450 "auto update doc" was introduced for helping users update docs like Operators.md and TestCoverage.md. It will be also useful that the CI can help users update backend node test as well.

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen added test CI pipelines Issues related to the CI pipeline utility labels Sep 20, 2023
@jcwchen jcwchen requested review from a team as code owners September 20, 2023 13:46
Copy link
Contributor

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

Potentially we should just update everything in CI because the CI machines are the most stable, compared to machines developers use?

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen
Copy link
Member Author

jcwchen commented Sep 20, 2023

Potentially we should just update everything in CI because the CI machines are the most stable, compared to machines developers use?

We have discussed this a lot in our offline meetings, but I think there are some concerns to make the update only happen in the CI. Let me conclude:

  • Operators.md is useful for PR reviewers by showing in the PR
  • Providing backend node test in the PR is useful for the PR author to verify them in their local

Also we have another option to remove Operators.md entirely: #5571. For now, I think making "auto update doc" update docs/backend test data is a fine temporary solution. PR authors can decide to update them from their own, or simply reply on the CI to update for them. Going forward we can discuss further and see whether we can eventually make these updates all happen in the CI.

@justinchuby
Copy link
Contributor

justinchuby commented Sep 20, 2023

Agree with all of the above. I meant we didn’t have to find the —diff and update. We can update all node tests (when ppl choose to use the auto gen label)

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby justinchuby added this pull request to the merge queue Sep 21, 2023
@jcwchen
Copy link
Member Author

jcwchen commented Sep 21, 2023

I meant we didn’t have to find the —diff and update. We can update all node tests (when ppl choose to use the auto gen label)

In that way some of irrelevant files (input.pb or output.pb) might be updated unintentionally, because NumPy behaves differently on different platforms. The CI is running on Linux and for those old data created from Windows will be changed in the CI. I think using "--diff" in CI makes more sense to let the generation tool only update those corresponding test data.

@gramalingam
Copy link
Contributor

Two points. (i) Extending the utility with the --diff option is a good thing, it will help even devs who want to do it locally for their own validation. (ii) Whether the CI should use that option or not is orthogonal. I am fine with using the --diff. (Ideally it should make no difference.)

Merged via the queue into onnx:main with commit a9e0a69 Sep 21, 2023
33 checks passed
@jcwchen jcwchen mentioned this pull request Sep 21, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI pipelines Issues related to the CI pipeline test utility
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants