Skip to content

Conversation

jorickert
Copy link
Collaborator

Some computeShape functions assume/rely on the operands being ranked.
This PR adds a check to them, so that they return failure in case this assumption does not hold, instead of asserting. This allows callers of this function to handle this failure

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@AlexandreEichenberger
Copy link
Collaborator

@jenkins-droid test this please

Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

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

LGTM,

I verified a few, let me just ask just to be sure. The return failure() that are added here are all in the computeShape(...) functions, which is fine since we expect to have successfully run shape inference before, were potentially unranked values have been at least ranked.

Thanks for adding "graceful" errors, much appreciated.

@jorickert
Copy link
Collaborator Author

@AlexandreEichenberger Yes, the return failure() are all added in computeShape . This allows callers of this function to decide how to handle failures in case computeShape fails because the input is unranked.

Before this PR, the functions computeShapeAndAssertOnFailure and computeShape would behave the same in many cases, as computeShape would assert internally.

It would be nice if you could merge this, as I do not have merge rights

@AlexandreEichenberger
Copy link
Collaborator

Will do, for some reason I don't have the usual button to update to the latest. do you mind doing this on your end, and then I will merge. I asked for you to have access right to run the CIs. After a PR or two, will give you merge permission too.

…n because of unraked operand types

Signed-off-by: Jonas Rickert <[email protected]>
@jorickert jorickert force-pushed the jrickert.upstream.unranked_shape_inference branch from b5b2892 to 06e4ca7 Compare December 7, 2024 10:52
@jorickert
Copy link
Collaborator Author

@AlexandreEichenberger I rebased onto the latest main

@AlexandreEichenberger AlexandreEichenberger merged commit 49fc9c1 into onnx:main Dec 9, 2024
7 checks passed
@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #15093 [push] Return a failure instead... started at 11:38

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #16066 [push] Return a failure instead... started at 11:22

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #16064 [push] Return a failure instead... started at 10:22

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #16064 [push] Return a failure instead... failed after 53 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #16066 [push] Return a failure instead... passed after 1 hr 27 min

@jorickert
Copy link
Collaborator Author

Jenkins Linux amd64 Build #16064 [push] Return a failure instead... failed after 53 min

@AlexandreEichenberger
The CI error looks like some issue with the CI setup itself to me:
s = '[0.002s][warning][perf,memops] Cannot use file /tmp/hsperfdata_root/232111 because it is locked by another process (e...QAAAAABAAAAAQEAAAABAQEBAAABAAABAQABAAEAAAEBAAEBAQABAQABAAAAAQEBAQEAAAABAQEBAQEAAQAAAAAAAQEAAAEAAAABAQAAAQEAAQEBAAEA"}]'

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #15093 [push] Return a failure instead... passed after 2 hr 18 min

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants