Introduce _VarInfo internally to reduce memory footprint in value propagation

[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?

[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.

[cbourjau]

I'm afraid running this PR against this branch in ndonnx (which makes extensive use of Spox's value propagation) gives rise to a lot of warnings of the type:

spox._exceptions.InferenceWarning:
 Output type for variable expanded of ai.onnx@21::Unsqueeze was not concrete - ValueError: Tensor int32[...] does not specify the shape -- in ?.

These warnings do not appear when running on spox from the main branch.

You can make pytest fail on these warnings in the ndonnx test suite by running it as follows:

pytest tests -W="error:Output type for variable"

Do you think you can take a look into what is going on here?

[neNasko1]

I'm afraid running this PR against this branch in ndonnx (which makes extensive use of Spox's value propagation) gives rise to a lot of warnings of the type:

spox._exceptions.InferenceWarning:
 Output type for variable expanded of ai.onnx@21::Unsqueeze was not concrete - ValueError: Tensor int32[...] does not specify the shape -- in ?.

These warnings do not appear when running on spox from the main branch.

You can make pytest fail on these warnings in the ndonnx test suite by running it as follows:

pytest tests -W="error:Output type for variable"

Do you think you can take a look into what is going on here?

This is caused by the fact that validate_types was being called before actually performing the value propagation.

Currently this type-validation is "optional", however AFAIK this is not really optional as validate is not exposed publicly. Should we deprecate this option.

[cbourjau]

Thanks for looking into the warnings on the ndonnx side! I think this PR is converging :). Could you add type hints to all new functions please?

[cbourjau]

Very nice! I have one very tiny nitpick, and then this is ready! Thank you very much!