Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enforce the validity of NIR graphs on creation #61

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stevenabreu7
Copy link
Collaborator

@stevenabreu7 stevenabreu7 commented Oct 12, 2023

PR #59 introduces code to check the consistency and "defined-ness" of input and output types across a NIR graph (also pointed out in #4). This is currently not enforced.

However, NIR, as an intermediate representation, would do well to enforce the correctness of all created NIR graphs. Otherwise, we end up with potentially many invalid NIR graphs that will effectively be un-parseable.

This PR enforces the validity of NIR graphs, as defined by the NIRGraph._check_types() method. If the graph types are undefined or inconsistent, a warning is printed and an attempt is made to infer them with NIRGraph.infer_types(). If this fails, an error is thrown.

TODO:

  • fix tests

Note: this can also be merged only after our paper deadline.

Copy link
Collaborator

@Jegp Jegp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a solid idea, but may I suggest adding a flag somewhere so the user can choose to ignore the pass? Maybe a parameter like ignore_type_check = False?

@stevenabreu7
Copy link
Collaborator Author

Good idea, I've added a flag as NIRGraph parameter (ensure_validity). I've also put a strict flag into the read and write functions for NIRGraphs. This is turned on by default (since people should be using this), but can be turned off.

Many tests are failing now, because we had a lot of inconsistent graph types defined in the tests..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants