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

test_view_dynamic_zero_dim no longer testing zero input #105066

Closed
ezyang opened this issue Jul 12, 2023 · 3 comments
Closed

test_view_dynamic_zero_dim no longer testing zero input #105066

ezyang opened this issue Jul 12, 2023 · 3 comments
Assignees
@titaiwangms
Labels
module: onnx Related to torch.onnx triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@ezyang
Copy link
Contributor

ezyang commented Jul 12, 2023

🐛 Describe the bug

#104828 (comment)

Filing an issue as requested by @thiagocrepaldi @titaiwangms

I don't think the exported model should work with a zero size input. The reason is that we 0/1 specialize on tracing; when you trace a model that has size 2, we assume that if you dynamically vary the shape, you won't vary it to 0 or 1. It is strange to have a model that exports and works with either size 0, or size 2.

If there is a more realistic example, we should put it in here instead.

Versions

main
@thiagocrepaldi thiagocrepaldi added the module: onnx Related to torch.onnx label Jul 12, 2023
@github-project-automation github-project-automation bot moved this to Inbox in ONNX Jul 12, 2023
@BowenBao
Copy link
Collaborator

iiuc the unittest's intention could be to op test scenarios from rcnn models where it may return zero boxes depending on input image. Based on recommendation, I suppose we should add a larger snippet that doesn't start for size 0 input but captures how one is emitted in the middle/output.

@ezyang Do you happen to know if such behavior from vision models is dynamically supported by symbolic tracing?

@titaiwangms titaiwangms self-assigned this Jul 12, 2023
@ezyang
Copy link
Contributor Author

ezyang commented Jul 12, 2023

Zero size input is legitimate, we sort of support this: if you ask whether or not something is equal to zero, we will always say False, but the hypothesis is the traced graph will work anyway for the zero case.

@soulitzer soulitzer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 14, 2023
@titaiwangms titaiwangms mentioned this issue Jul 25, 2023
Closed
titaiwangms added a commit that referenced this issue Jul 26, 2023
@titaiwangms
Update base for Update on "[ONNX] Add comment on test_view_dynamic_ze…
fe7f448 
…ro_dim"


From #105066. 

The case was meant to test 0 bbox generated by vision models, but the bboxes still have `.view()` operated. The case was disabled, and not supported. Added a comment to clear the potential confusion in the future. We will wait for model example to proceed on this.

[ghstack-poisoned]
titaiwangms added a commit that referenced this issue Jul 26, 2023
@titaiwangms
Update on "[ONNX] Add comment on test_view_dynamic_zero_dim"
43084f1 
From #105066. 

The case was meant to test 0 bbox generated by vision models, but the bboxes still have `.view()` operated. The case was disabled, and not supported. Added a comment to clear the potential confusion in the future. We will wait for model example to proceed on this.

[ghstack-poisoned]
pytorchmergebot pushed a commit that referenced this issue Jul 26, 2023
@titaiwangms @pytorchmergebot
[ONNX] Add comment on test_view_dynamic_zero_dim (#105950)
45322fa 
From #105066.

The case was meant to test 0 bbox generated by vision models, but the bboxes still have `.view()` operated. The case was disabled, and not supported. Added a comment to clear the potential confusion in the future. We will wait for model example to proceed on this.
Pull Request resolved: #105950
Approved by: https://github.com/BowenBao, https://github.com/thiagocrepaldi
pytorchmergebot pushed a commit that referenced this issue Jul 26, 2023
@titaiwangms
Update base for Update on "[ONNX] Add comment on test_view_dynamic_ze…
825ac29 
…ro_dim"


From #105066. 

The case was meant to test 0 bbox generated by vision models, but the bboxes still have `.view()` operated. The case was disabled, and not supported. Added a comment to clear the potential confusion in the future. We will wait for model example to proceed on this.

[ghstack-poisoned]
pytorchmergebot pushed a commit that referenced this issue Jul 26, 2023
@titaiwangms
Update on "[ONNX] Add comment on test_view_dynamic_zero_dim"
e508c36 
From #105066. 

The case was meant to test 0 bbox generated by vision models, but the bboxes still have `.view()` operated. The case was disabled, and not supported. Added a comment to clear the potential confusion in the future. We will wait for model example to proceed on this.

[ghstack-poisoned]
@titaiwangms
Copy link
Collaborator

Re-enable in #127349

@github-project-automation github-project-automation bot moved this from Inbox to Done in ONNX May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@titaiwangms titaiwangms

Labels
module: onnx Related to torch.onnx triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
ONNX
Status: Done
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ezyang @thiagocrepaldi @BowenBao @soulitzer @titaiwangms