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

Use frozen NumPy array for ImageDecoder backend test if no Pillow #5511

Merged
merged 25 commits into from
Sep 7, 2023

Conversation

jcwchen
Copy link
Member

@jcwchen jcwchen commented Aug 18, 2023

Description

  • Use frozen NumPy array for ImageDecoder backend test's input/output if there is no installed pillow
  • Test with and without pillow in CIs when running test generation

Motivation and Context

Similar to #5498. Hopefully this PR can entirely fix #5491.

Ideally test generation should be as simple as possible without other new dependency especially that dependency was not used in other places (reference will only use pillow when needed). Let's merge this PR first and later I will have another PR to freeze input/output as a pure NumPy array and simply keep generate_checkerboard/generate_test_data for future reference.

…opencv-python

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen added the test label Aug 18, 2023
@jcwchen jcwchen requested a review from a team as a code owner August 18, 2023 00:30
@jcwchen jcwchen added the run release CIs Use this label to trigger release tests in CI label Aug 18, 2023
Copy link
Member Author

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

However, these test data are actually image so the array is quite big. They look very unorganized in the introduced Python file: image_decoder_data.py with 35637 lines... If anyone has better idea to handle them, feel free to let me know. Thanks!

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@justinchuby
Copy link
Contributor

Can we use smaller images? Make just a few pixels?

@lutzroeder
Copy link
Member

@jcwchen validated this branch fixes the underlying issue. collect_snippets runs without requiring cv2.

Co-authored-by: Aditya Goel <48102515+adityagoel4512@users.noreply.github.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Copy link
Member Author

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

Can we use smaller images? Make just a few pixels?

Good question. Now by default generate_test_data is using 40x40, which is sort of in the middle, but I don't have proper number in mind. Ideally the size of image should be large enough for runtime to test. @jantonguirao do you think it's a good idea to shrink the test image a bit?

@jcwchen
Copy link
Member Author

jcwchen commented Aug 21, 2023

@jcwchen validated this branch fixes the underlying issue. collect_snippets runs without requiring cv2.

@lutzroeder Great! Thank you for the validation.

@jantonguirao
Copy link
Contributor

@jcwchen We could reduce the image size to something like 32x32, but I wouldn't go much smaller than that.

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen changed the title Use freeze NumPy array for ImageDecoder backend test without cv2 Use frozen NumPy array for ImageDecoder backend test if no cv2 Aug 22, 2023
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen requested a review from a team as a code owner August 22, 2023 16:28
Copy link
Member Author

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

@jcwchen We could reduce the image size to something like 32x32, but I wouldn't go much smaller than that.

Thank you for the suggestion! I have used 32x32 instead of 40x40 and now the used lines in that Python file was reduced from 35637 lines to 23,604 lines.

@xadupre
Copy link
Contributor

xadupre commented Aug 28, 2023

The proposal seems rather complex to me. Why not using pillow instead of opencv?

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

jcwchen commented Aug 29, 2023

As @justinchuby suggested, I turned off black formatter for image_decoder_data.py and rearranged those large NumPy arrays. Now the code lines was decreased from 23,604 to 5,134, which is now previewable in this PR. @onnx/sig-operators-approvers PTAL. Thanks!

@jcwchen jcwchen added the review needed: operators approvers Require reviews from members of operators-approvers label Sep 1, 2023
@justinchuby
Copy link
Contributor

Would be nice to wait for #5555 because there are slight changes in values

@justinchuby justinchuby added this pull request to the merge queue Sep 5, 2023
@justinchuby justinchuby removed this pull request from the merge queue due to a manual request Sep 5, 2023
@jcwchen jcwchen changed the title Use frozen NumPy array for ImageDecoder backend test if no cv2 Use frozen NumPy array for ImageDecoder backend test if no Pillow Sep 6, 2023
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>

# Conflicts:
#	docs/Operators.md
#	docs/TestCoverage.md
#	onnx/backend/test/case/node/image_decoder.py
#	onnx/backend/test/data/node/test_image_decoder_decode_bmp_rgb/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg2k_rgb/model.onnx
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg2k_rgb/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg2k_rgb/test_data_set_0/output_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_bgr/model.onnx
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_bgr/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_bgr/test_data_set_0/output_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_grayscale/model.onnx
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_grayscale/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_grayscale/test_data_set_0/output_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_rgb/model.onnx
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_rgb/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_jpeg_rgb/test_data_set_0/output_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_png_rgb/model.onnx
#	onnx/backend/test/data/node/test_image_decoder_decode_png_rgb/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_tiff_rgb/model.onnx
#	onnx/backend/test/data/node/test_image_decoder_decode_tiff_rgb/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_webp_rgb/model.onnx
#	onnx/backend/test/data/node/test_image_decoder_decode_webp_rgb/test_data_set_0/input_0.pb
#	onnx/backend/test/data/node/test_image_decoder_decode_webp_rgb/test_data_set_0/output_0.pb
@justinchuby justinchuby removed the review needed: operators approvers Require reviews from members of operators-approvers label Sep 6, 2023
@jcwchen jcwchen changed the title Use frozen NumPy array for ImageDecoder backend test if no Pillow [WIP] Use frozen NumPy array for ImageDecoder backend test if no Pillow Sep 6, 2023
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen changed the title [WIP] Use frozen NumPy array for ImageDecoder backend test if no Pillow Use frozen NumPy array for ImageDecoder backend test if no Pillow Sep 6, 2023
@jcwchen
Copy link
Member Author

jcwchen commented Sep 6, 2023

I have updated _image_decoder_data.py accordingly with the new data from #5555 (created from Pillow). Also CI pipeline will test with/without pillow with node test generation.

If no one has other concern for this PR, I will forward this PR by the end of today. Thank you all for the reviews!

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen added this pull request to the merge queue Sep 6, 2023
Merged via the queue into onnx:main with commit 70d146a Sep 7, 2023
53 checks passed
@jcwchen jcwchen deleted the jcw/freeze-cv2-input branch September 7, 2023 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run release CIs Use this label to trigger release tests in CI test
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

collect_snippets requires cv2 dependency not available on macOS
7 participants