Split value prop

[neNasko1]

Fixes #187 by making the current user-facing Var be a wrapper around a helper class VarInfo, where Var is a pair (VarInfo, PropValue).

The idea is to have VarInfo be the object that is passed around inside of all functions and be pointed to by all Node-s, so concrete propagated values can be garbage collected correctly.

Checklist for implementation

• Provide better typing around input_prop_values dictionary
• Fix adapt_node
• Fix documentation
• Add a CHANGELOG.rst entry
• Fix Var-s passed to manual inference
• Make tests passing

Breaking changes checklist

• Examine the performance implications?
• Examine downstream repos, does the change brake anything we rely on?

[cbourjau]

Thanks for taking this on and great work to already make the tests pass! I am trying to grok the details of the implementation. While doing so I already left a few high-level comments. I am not quite convinced that the use of a meta class is buying us much with respect to the introduced complexity/scariness of it. Would you mind adding type hints for its use and/or point out where it would provide a mypy-testable benefit compared to a simpler/dynamic structure such as a dict[str, Var]s?

[cbourjau]

It is a bit of a pitty that we don't have any output type here. Having a type such as dict[str, Var] would be better than nothing. I'm not sure how we could type this properly using the TypeBaseVars. If we lose the type information already here, is the TypeBaseVars-complexity really worth the effort?

[jbachurski]

Are you sure there isn’t a cleaner way of doing this? Or perhaps some simpler hack. It feels like a significant change.
I wonder if your standpoint on value propagation changed, Christian @cbourjau - it was mostly intended as an experimental side-feature to improve experimentation back when I added it, not something that should be relied on in live systems. So I’m not sure a large change is worth it to improve the GC behaviour.
I’m afraid I won’t be able to help review this as I’m pretty starved on time.

[cbourjau]

Thanks for your feedback @jbachurski ! Let me provide a little more context.

I wonder if your standpoint on value propagation changed, Christian ?

Yes, it did indeed! ndonnx makes it very easy to try out NumPy code with constant arrays. So much so that it is perfectly reasonable to quickly convert large NumPy arrays into ndonnx ones and to throw them at our code. While technically still a "debugging" feature, it is now used very commonly during development and on very large graphs. The value propagation in ndonnx is currently re-implemented there, but I believe it would be cleaner to do it properly here in Spox on the operator level.

[jbachurski]

While technically still a "debugging" feature, it is now used very commonly during development and on very large graphs. The value propagation in ndonnx is currently re-implemented there, but I believe it would be cleaner to do it properly here in Spox on the operator level.

Curious. Yes, I noticed that it was early on and found it surprising, but I didn't want to question it :)
I imagine it would be cleaner like so indeed – good luck 🍀

I'll let you know in case I come up with something accidentally but yeah, it's unfortunately an extremely busy period for me to think over this design aspect.