[jit] improve error message on recursive class defs #21842
Conversation
We don't support this yet, so it should error clearly.
[jit] improve error message on recursive class defs We don't support this yet, so it should error clearly. gh-metadata: pytorch pytorch 21842 gh/suo/63/head
test/test_jit.py
Outdated
| """ | ||
| Recursive class types not yet supported. We should give a good error message. | ||
| """ | ||
| with self.assertRaisesRegex(RuntimeError, "Recursive class attributes not yet supported"): |
There was a problem hiding this comment.
"Recursive class" is a bit vague as a concept can it be made more clear what is being objected to here?
| // Recursively check contained types. We need to do this because a user may do | ||
| // A -> B -> A. | ||
| for (const auto& type : attrType->containedTypes()) { | ||
| if (isRecursive(classType, type)) { |
There was a problem hiding this comment.
The naive search here seems like it could get really inefficient with any complicated hierarchies
There was a problem hiding this comment.
BFS would be faster given how types are structured. But like, how complicated are we talking lol—this search is linear in the size of the type DAG, so unless someone is making 100000-nested compound types I think we're ok.
There was a problem hiding this comment.
I guess we're doing a second traversal if it's a tuple type, so if we have deep nested tuples we may need to add manually unrolling.
There was a problem hiding this comment.
I think this is exponential in the worst case... Edit: nvm not exponential i think it's n^3
There was a problem hiding this comment.
If you have N numbered nodes where each node is connected to all nodes with a lower number
[jit] improve error message on recursive class defs We don't support this yet, so it should error clearly. gh-metadata: pytorch pytorch 21842 gh/suo/63/head
[jit] improve error message on recursive class defs We don't support this yet, so it should error clearly. gh-metadata: pytorch pytorch 21842 gh/suo/63/head
[jit] improve error message on recursive class defs We don't support this yet, so it should error clearly. gh-metadata: pytorch pytorch 21842 gh/suo/63/head
Stack from ghstack:
We don't support this yet, so it should error clearly.
Differential Revision: D15857568