[BUG] @tensorclass-decorated dataclasses violate @beartype-based runtime type-checking 😭
#1243
Comments
|
Hey! Can I suggest a more neutral and direct style next time? |
Sure. That's cool, I guess? But... what about
Obviously, ignoring stuff that should be type-checked isn't great. I suppose my next question would inevitably be:
As currently defined, users can't even annotate functions, attributes, or variables that accept any arbitrary def use_tensorclass(tensorclass: object) -> object: ...Ideally, the Are
Possibly ideal, but pragmatically infeasible. I'm autistic. Intensely autistic. I live in a remote cabin in the Canadian wilderness. Imagine the most neurodivergent INTP you've ever met (or possibly are). Then magnify and ramify that neurodivergence by the exponentiated power of Guido. That's why I do @beartype. It's like autism personified. 😅
tl;dr:
Guh! My apologies. I absolutely don't intend offense. I just have little time to attend to third-party integration issues like this and wish that I mean, technically they're classes. Sure. But nobody else in the Python ecosystem can identify or use them. Oh, well. Whatevaaahs. Definitely not here to argue. 2025 is already hard enough for Planet Earth. No need to harden matters even further with petty argumentation. I sigh. 😩 |
|
Okay. I dug deep into the TensorDict codebase. The type-checking violation that @beartype currently raises when confronted with It's TensorDict. @beartype is correct. There are critical unresolved issues in the Let's start with the obvious: "What is @beartype complaining about now?" TensorDict Is Shadowing Builtin TypesUnfortunately, TensorDict has committed the cardinal sin of Pythonic API design. TensorDict is shadowing builtin types. In fact, TensorDict isn't shadowing just a single builtin type. TensorDict is shadowing many builtin types. This includes Python's core C-based scalar types: Shadowing builtin types promotes obscure bugs that are non-trivial to identify, let alone debug. Shadowing builtin types is always an abysmal idea. This is no exception. To demonstrate exactly why TensorDict's shadowing of builtin types is so problematic, I'd like to first exhibit a simple thought experiment: a class declaring just two class attributes in a manner vaguely similar to the class OhGods(object):
def bool(self) -> bool: return False
def __eq__(self, other: object) -> bool: return False
Yes. Tragically, it does: # What is the return type hint annotating the bool() method defined above?
>>> OhGods.bool.__annotations__['return']
<class 'bool'> # <-- *GOOD.* That's exactly what we expect.
# What is the return type hint annotating the __eq__() method defined above?
>>> OhGods.__eq__.__annotations__['return']
<function OhGods.bool at 0x7fd72d75c360> # <-- *HOLY SHIT WTFFFFFFFFFFFF!*As expected, the return type hint annotating the Indeed, it is. Since the But wait! You're now thinking:
Indeed, your thoughts would have been correct. Sadly... TensorDict Enabled PEP 563, The Ultimate Typing SatanEvery TensorDict submodule leads with Generally speaking, PEP 563 should never be enabled. There are no genuine use cases for doing so and innumerable use cases for not doing so. This is the latter. By enabling PEP 563, TensorDict is instructing type-checkers to defer the evaluation of its type hints to their point of use rather than their point of declaration. Notably: # PEP 563 implicitly transforms this declaration of your _eq() function...
def _eq(self, other: object) -> bool:
# ...into this equivalent declaration whose type hints are all stringified.
def _eq(self, other: 'object') -> 'bool':You monkey-patch the above globally scoped Type-checkers then resolve that stringified return type hint by dynamically evaluating that hint within the lexical context of the body of that Because you have enabled PEP 563, all of the above applies to TensorDict. Woops. Clearly, methods are not valid type hints. When confronted with invalid type hints, type-checkers raise exceptions. This is exactly what @beartype is doing when confronted with TensorDict has committed the cardinal sin of Pythonic API design. Can TensorDict repent of its sins? Numerous solutions exist. Let's examine a few. Solution 1: Stop Shadowing Builtin TypesThe most obvious solution is to simply stop shadowing builtin types. Consider NumPy, a comparable tensor-centric API. NumPy intentionally avoids shadowing builtin types by suffixing NumPy-specific types sharing similar names as those of builtin types with underscore characters. This disambiguation avoids conflicts, ambiguities, and type shadowing. The This obvious solution is also the most painful solution from the perspective of downstream consumers and reverse dependencies, who would correctly consider this to be a backward compatibility-breaking change. One responsible approach would be to:
But suppose you hate the most obvious solution. Is there anything else you can do? There is plenty. Consider, for example... Solution 3: Disable PEP 563Removing the No idea. Maybe. Maybe not. It's kinda hard to say. PEPs 649 and 749 are still moving targets subject to vitriolic (and I do mean vitriolic) ongoing debate. Under PEPs 649 and 749, Python ≥ 3.14 will evaluate annotations lazily – just like under PEP 563. It's probably best not to assume that annotations will be evaluated in a global scope as you'd like them to be. Which leads us to... Solution 3: Stop Annotating Methods with Shadowed TypesThis is a weird one. Yet, it might actually work. Instead of annotating methods as returning the shadowed # Yes, this is insane. Yes, this *MIGHT* just work.
bool_ = bool
def _eq(self, other: object) -> bool_: # <-- note "bool_" rather than "bool"Of course, that's completely untested. Still, that's insane enough that it might just work. The disadvantages are numerous, however:
Even worse solutions exist, including... Solution 4: Just Remove All Your Type Hints, Because They're BustedSeriously. This is an awful solution – but it's better than what TensorDict currently is. TensorDict currently resides in the Uncanny QA Valley: you've annotated a lot of different things with type hints everywhere, but you haven't actually tested any of those type hints with type-checking anywhere. The result? Massive issues. Are you aware of just how many type-checking errors TensorDict currently has? It's bad. It's beyond bad. I couldn't believe my eyes when I saw just how bad the existing QA situation in TensorDict is. I've never seen something similar. I don't even know how you'd begin to go about repairing this badness. Maybe just burn the entire thing down to the ground and start over fresh? No idea. But this is sheer madness: $ pwd
/tmp/tensordict
$ ls
_C.so _lazy.py _reductions.py _unbatched.py nn return_types.py utils.py
__init__.py _nestedkey.py _td.py base.py persistent.py tensorclass.py version.py
__pycache__ _nestedkey.pyi _tensordict functional.py prototype tensorclass.pyi
_contextlib.py _pytree.py _torch_func.py memmap.py py.typed tensordict.py
$ mypy .
...
Found 2148 errors in 29 files (checked 36 source files)
$ pyright
...
2701 errors, 493 warnings, 0 informationsSeriously. @beartype bailed out on the first of these errors. Perhaps TensorDict should do so, too. The only thing worse than no type hints is untested type hints. If you remove all of the type hints across the entire TensorDict codebase, then runtime type-checking issues go away. It's magical, but also horrible. Obviously, nobody wants to remove type hints. It took you time and effort to wrote those type hints. You're certainly not walking that back. Right? No worries. I agree. Still, open-source volunteerism is only finite. We can't QA everything. We Hate All of Your Solutions...yeah. Kinda thought you might say that. From my perspective, @beartype is at a bit of a loss as to what to do with TensorDict. We either:
There's a lot for me to chew on here. Unsurprisingly, I'm currently leaning towards the latter: bitterly complain about TensorDict QA but otherwise avoid type-checking anything in TensorDict. Thanks so much for reading this far. Of course, nobody read this far. In the unlikely chance that someone did read this far, I humbly apologize for squandering your entire week. We should probably have just played video games instead. At least I'd finally have defeated the Dread Lord Albalos then! 😄 |
|
We appreciate your feedback, but we currently have other priorities that need to be addressed. We may revisit this issue in the future. If you want to help us solve these pain points, feel free to submit small scale PRs! This is an open-source project, contributions are more than welcome. |
Hybrid runtime-static type-checking @beartype maintainer @leycec here. Because we live on the worst timeline, @beartype users like @pablovela5620 pour one out for Austin Texas are now futilely attempting to integrate @beartype with
@tensorclass-decorated types.Predictably, @beartype refuses. Minimal-working example or it didn't happen:
...which raises the unreadable exception:
I'm... outta my depth here, guys. There's so much badness happening all it once that it's non-trivial to track all of the badness.
Let's start with the obvious.
TensorDict Return Type Hints Appear to be Madness Incarnate
The tensordict.tensorclass._eq() method appears to be annotated by a return type hint that is (...waitforit) a
<function _wrap_td_method.<locals>.wrapped_func at 0x726f56d64e00>closure function.No idea, guys. @beartype doesn't generally lie about these things. Very well. I admit it. @beartype often lies. So, that's possibly what is happening. Alternately, @beartype could be as confused as we all are right now. Let's generously assume that what @beartype is thinking is happening is happening. It's unclear why anybody would want that to happen, though. Also, this is why type-checking actually is important. It's not something that TensorDict should be intentionally ignoring. Type-checking could have caught issues like this earlier.
Because something definitely smells in the TensorDict codebase. Very well. It might be the @beartype codebase that actually smells here. Functions aren't valid type hints, obviously. So why is TensorDict possibly using functions as type hints? Alternately, why does @beartype mistakenly believe that this is what is happening?
No idea, guys. It's probably just a trivial typo somewhere five layers deep in the bowels of some private submodule dynamically defined in-memory, just because. Still, this is badness.
Which leads us to...
type(MyData) is typeThe type of
@tensorclass-decorated dataclasses inexplicably appears to be (...waitforit) the ambiguoustypesuperclass that conveys no meaningful semantics:Uhh. Wat? Because these dataclasses inherit from no TensorDict-specific superclasses or metaclasses, downstream third-party consumers like @beartype have no sane means of detecting and thus supporting these dataclasses at runtime.
Like, seriously. Do something to distinguish the method-resolution orders (MROs) of these dataclasses from standard
@dataclasses.dataclass-decorated dataclasses. Doing nothing definitely isn't cutting it.Which leads us to...
That Ship Has Already Sailed
So. I get it. The
@tensorclassship has already sailed. There's already millions of lines of code in the wild using this API. @beartype can hope for a better API all it likes – but that doesn't particularly change the grim reality of the situation on the ground.So. How should downstream third-party consumers like @beartype detect these dataclasses then? I note a public
tensordict.is_tensorclass()tester function that seems particularly pertinent. The issue there, of course, is that calling that function requires @beartype to import that function which requires @beartype to then require TensorDict as a mandatory dependency – which @beartype is absolutely not doing. @beartype has no mandatory dependencies and that's never changing.So, What now? Even importing TensorDict with
import tensordictand then immediately terminating the active Python process underpython -vshows an extreme number of extremely costly imports. In other words, @beartype is absolutely also not doing something like this:Although trivial, that would make importing @beartype itself equally costly. The whole point of @beartype is to be "cost-free." Which leads us to...
dir(MyData)is Truly a Nightmare on EarthSeriously. I cannot believe how utterly intense the
TensorDictAPI is. This is the most outrageously verbosedir()output I've ever seen Python spit out:I mean... just... WTTTTTTTTTTTTTTTTF!? There's even an attribute called
cumminup there, which just raises even more questions than it answers. 🤣I genuinely have no idea where to start with this beastly nightmare. Technically, I do note a private
_is_tensorclassattribute in the above output. The existence of that attribute definitely suggests the current type to be atensordict.tensorclass. There's no guarantee of that, though. Other third-party types in the wild might very well define the same private_is_tensorclassattribute.But... I guess that's what I gotta roll with, huh? Sucks, guys. This pretty much sucks.
So. Are You Actually Suggesting Anything Useful?
Yeah. I'm not here to just complain. That's only one of the reasons I'm here. 😉
Basically, feature request #663 really needs to happen to make TensorDict usable at runtime by everybody else in the Python ecosystem. If #663 happens, then @beartype can just trivially compare the
obj.__class__.__module__andobj.__class__.__name__dunder attributes guaranteed to exist on any arbitrary object to decide whether that object is a TensorDict dataclass or not.This trivial test just operates on strings and thus doesn't require @beartype to import TensorDict in a global scope: e.g.,
Until that glorious release day happens, I guess the best we can do is (as suggested above):
That's... not great. But that's what we rollin' with. I sigh! I sigh so hard. 😩
The text was updated successfully, but these errors were encountered: