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.

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

Validate the generated number of input.pb and output.pb and new node files in CIs #4514

Merged
merged 15 commits into from Oct 3, 2022
Merged

Validate the generated number of input.pb and output.pb and new node files in CIs #4514

merged 15 commits into from Oct 3, 2022

Conversation

jcwchen
Copy link
Member

@jcwchen jcwchen commented Sep 15, 2022 •
edited

Description

  • Validate the generated number of input.pb and output.pb in CIs -- added, deleted, renamed for output_.pb and input_.pb will be detected.
  • Introduce --clean in generate_data tool for CI use to clean-up existing node directory. Now CIs will detect unreproducible tests, which means they cannot be generated by existing node test scripts.
    • This PR also removes existing unreproducible tests, such as test_basic_convinteger, test_convtranspose_with_kernel, test_depthtospace_crd_mode, test_depthtospace_dcr_mode, test_pow_types_float, test_pow_types_int, test_reduce_log_sum, test_reduce_sum_empty_axes_input_noop_random, test_unsqeeze_axis_3. They are generated by old removed test script and cannot be reproduced anymore.
  • Mention in AddNewOp.md about how to fix this kind of issue by adding --clean. The tool will throw a warning if it detects there are other model directories if they cannot be reproduced.

Motivation and Context

Some validation will be missing after #4431. We should additionally handle the validation of generated number of input.pb and output.pb

@jcwchen
validate the generated number of input.pb and output.pb in CIs
0ec3d7a 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen requested a review from a team as a code owner September 15, 2022 14:10
@jcwchen
--diff-filter=ADR
9402ee5 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen added CI pipelines Issues related to the CI pipeline test labels Sep 15, 2022
jcwchen
jcwchen commented Sep 20, 2022
View reviewed changes
.azure-pipelines/Windows-CI.yml
IF NOT %ERRORLEVEL% EQU 0 (
@echo "git diff for test generation returned failures. Please check updated node test files"
EXIT 1
)
git diff --exit-code --diff-filter=ADR -- . :!onnx/onnx-data.proto :!onnx/onnx-data.proto3
Copy link
Member Author

Choose a reason for hiding this comment

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

Added, deleted, renamed for *output_*.pb and *input_*.pb will be detected.

@jcwchen
add --clean for generate-data
0b90614 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen
Merge branch 'jcw/validate-i/o' of https://github.com/jcwchen/onnx in…
74439e4 
…to jcw/validate-i/o
@jcwchen
add --clean in CIs
cd16d5b 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen requested a review from a team as a code owner September 28, 2022 15:21
@jcwchen
remove unreproducible tests
d54a8c4 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen changed the title Validate the generated number of input.pb and output.pb in CIs Validate the generated number of input.pb and output.pb and new node files in CIs Sep 28, 2022
@jcwchen jcwchen mentioned this pull request Sep 28, 2022
Merged
p-wysocki
p-wysocki approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@p-wysocki p-wysocki left a comment

Choose a reason for hiding this comment

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

I think it's a great addition and this check can save a lot of development time, especially for new contributors. I had issues myself at #4481, which would be easily detected by this check.

jcwchen
jcwchen commented Sep 28, 2022
View reviewed changes
Copy link
Member Author

@jcwchen jcwchen left a comment •
edited

Choose a reason for hiding this comment

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

.

@jcwchen
add sentences in AddNewOp.md
fea14d5 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen
Copy link
Member Author

jcwchen commented Sep 29, 2022

Not sure whether we can directly add --clean in the script:

onnx/tools/update_doc.sh

Line 25 in de24599

python onnx/backend/test/cmd_tools.py generate-data

Or even set --clean=True by default.

@gramalingam
Copy link
Contributor

Not sure whether we can directly add --clean in the script:

onnx/tools/update_doc.sh

Line 25 in de24599

python onnx/backend/test/cmd_tools.py generate-data

Or even set --clean=True by default.

Would it be worthwhile for the doc generator to track files it generated and finally issue a warning message if there were old files (not generated in this run)?

@jcwchen
check original directory number
78fb4d2 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen
Merge branch 'main' into jcw/validate-i/o
86efcea
@jcwchen
typo
c4d14bc 
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen
Copy link
Member Author

jcwchen commented Sep 29, 2022

Would it be worthwhile for the doc generator to track files it generated and finally issue a warning message if there were old files (not generated in this run)?

Makes sense to me. I have added such a check in cmd_tools and if there are additional files, it will throw a warning to let users understand. Please review it. Thanks!

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

jcwchen commented Oct 3, 2022

@gramalingam if you have time, could you please review this PR? (It needs an approval from SIG-operators-approvers as well). Thank you!

@gramalingam
Copy link
Contributor

So, all the deleted *.pb files were spurious (out-dated) files created by this issue? Good to catch them and remove them!

@jcwchen
Copy link
Member Author

jcwchen commented Oct 3, 2022 •
edited

So, all the deleted *.pb files were spurious (out-dated) files created by this issue? Good to catch them and remove them!

Yes, they were caught by the enhanced CI check (with --clean) brought by this PR because they cannot be regenerated by any existing test script. Therefore I removed them.

@jcwchen jcwchen enabled auto-merge (squash) October 3, 2022 15:33
@jcwchen jcwchen merged commit 4ccc72b into onnx:main Oct 3, 2022
@jcwchen jcwchen deleted the jcw/validate-i/o branch October 3, 2022 15:42
broune pushed a commit to broune/onnx that referenced this pull request May 6, 2023
@jcwchen
Validate the generated number of input.pb and output.pb and new node …
95ad838 
…files in CIs (onnx#4514)

* validate the generated number of input.pb and output.pb in CIs

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* --diff-filter=ADR

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* add --clean for generate-data

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* add --clean in CIs

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* remove unreproducible tests

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* add sentences in AddNewOp.md

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* check original directory number

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* typo

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

* nit: warning msg

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liqunfu liqunfu liqunfu approved these changes

@gramalingam gramalingam gramalingam approved these changes

@p-wysocki p-wysocki p-wysocki approved these changes

Assignees
No one assigned
Labels
CI pipelines Issues related to the CI pipeline test
Projects
PR Tracker
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jcwchen @gramalingam @liqunfu @p-wysocki