Repository navigation
Enhanced type inference and checking, called from NIRGraph.__init__ - #153
Merged
fabio-innatera merged 12 commits intoJul 7, 2025
Merged
Conversation
fabio-innatera
force-pushed
the
type-inference-and-check
branch
from
June 23, 2025 15:02
919772e to
fe2868d
Compare
fabio-innatera
force-pushed
the
type-inference-and-check
branch
from
June 23, 2025 17:27
fe2868d to
8f108df
Compare
…lid graphs otherwise
fabio-innatera
force-pushed
the
type-inference-and-check
branch
from
June 23, 2025 18:06
8f108df to
76f2032
Compare
fabio-innatera
marked this pull request as ready for review
June 24, 2025 10:33
Jegp
approved these changes
Jun 24, 2025
Jegp
left a comment
Collaborator
There was a problem hiding this comment.
Excellent work! I really appreciate the thought you put into this and the number of tests you added to cover this.
I have one comment around the naming for _forward_type_inference that I'd like your opinion on. Apart from that, I couldn't find any problems. Nice!
You bring a good point that this may mess up old graphs/weirdly shaped graphs. But you neatly wrapped the inference + type checking in the constructor, so I still think it should be possible to build the graphs and ignore the type checking. I retain that the checking should be default and that any choice to ignore the typing should be a conscious choice. So, I honestly like this model.
This was referenced Jun 25, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is my first stab at doing type inference, in addition to type checking, on graph construction (
NIRGraph.__init__).I believe this PR will lead to some more ecosystem breakage, but in many cases I think it should improve the situation of introducing the
type_checkparameter recently, because many graphs should now be valid after running type inference automatically on construction.Notes:
Input and Output node handling
We automatically add Input and Output nodes if they are missing from the NIRGraph. Before, we only did this on NIRGraph.from_list, but we now do this on all graphs. This is a requirement for nested graphs, otherwise we can't validate the outputs from nested graphs against the data going in and out of it from the outer graph.
A major break is the requirement of Input and Output nodes to be present for graphs that have no non-ambiguous inputs or outputs. This is the case for certain recurrent connections, for example:
For this graph, it's impossible to know which one is the input/source node and which one is the output/sink node, so the type inference will throw an error.
This graph with a recurrent connection is fine: