Integrate Automated QDQ placement tool - Part 1 #701
Conversation
Signed-off-by: Will Guo <[email protected]>
willg-nv
force-pushed
the
dev-willg-integrate-auto-qdq-placement-part1
branch
from
9c53783 to
f872e70
Compare
|
Hi @gcunhase, could you help me review this PR? thanks! |
| self.children.append(child) | ||
| child.set_parent(self) | ||
|
|
||
| def _is_descendant_of(self, potential_ancestor: "Region") -> bool: |
There was a problem hiding this comment.
This seems like a handy util. Do we want to keep this public?
| """ | ||
| return self.nodes | ||
|
|
||
| def get_all_nodes_recursive(self, _visited: set[int] | None = None) -> set[int]: |
There was a problem hiding this comment.
nit: can be renamed to get_region_nodes_and_descendants()
| # Detect cycles | ||
| if self.id in _visited: | ||
| logger.warning(f"Cycle detected in region {self.id} during node traversal") | ||
| return set() |
There was a problem hiding this comment.
add_child() alaready prevents adding cycles in the graph. So I think this check is redundant.
There was a problem hiding this comment.
I think this check could help avoid infinite loop when graph editing encountered cycle is unexpectedly. I think this lines could be replaced with an assert statement which could make debug easier.
| logger.warning(f"Cycle detected in region {self.id} during node traversal") | ||
| return set() | ||
|
|
||
| _visited.add(self.id) |
There was a problem hiding this comment.
We can remove _visited in that case too.
| # Detect cycles | ||
| if self.id in _visited: | ||
| return False |
There was a problem hiding this comment.
visited and cycle detection logic can be removed from here too.
| def run_tests(): | ||
| """Run all insertion point and scheme tests.""" | ||
| print("=" * 70) | ||
| print("Autotuner Insertion Points & Schemes Test Suite") | ||
| print("=" * 70) | ||
|
|
||
| # Create test suite | ||
| loader = unittest.TestLoader() | ||
| suite = unittest.TestSuite() | ||
|
|
||
| # Add test classes | ||
| suite.addTests(loader.loadTestsFromTestCase(TestNodeInputInsertionPoint)) | ||
| suite.addTests(loader.loadTestsFromTestCase(TestRegionOutputInsertionPoint)) | ||
| suite.addTests(loader.loadTestsFromTestCase(TestChildRegionInputInsertionPoint)) | ||
| suite.addTests(loader.loadTestsFromTestCase(TestInsertionScheme)) | ||
|
|
||
| # Run with verbose output | ||
| runner = unittest.TextTestRunner(verbosity=2) | ||
| result = runner.run(suite) | ||
|
|
||
| # Summary | ||
| print("\n" + "=" * 70) | ||
| print("Test Summary") | ||
| print("=" * 70) | ||
| print(f"Tests run: {result.testsRun}") | ||
| print(f"Successes: {result.testsRun - len(result.failures) - len(result.errors)}") | ||
| print(f"Failures: {len(result.failures)}") | ||
| print(f"Errors: {len(result.errors)}") | ||
|
|
||
| if result.wasSuccessful(): | ||
| print("\n✓ All insertion point and scheme tests passed!") | ||
| return 0 | ||
| else: | ||
| print("\n✗ Some tests failed") | ||
| return 1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(run_tests()) |
There was a problem hiding this comment.
Please remove this boilerplate code. You can refer to the other tests to see how they are called with pytest.
| assert region.get_type() == RegionType.LEAF | ||
| assert region.get_parent() is None | ||
| assert len(region.get_children()) == 0 | ||
| print("✓ LEAF region creation") |
There was a problem hiding this comment.
Remove the print statements from this file too
| class TestRegion(unittest.TestCase): | ||
| """Test Region class functionality.""" | ||
|
|
||
| def test_leaf_region_creation(self): |
There was a problem hiding this comment.
The three region creation tests are almost identical. It would be great if you could parametrize them.
| def test_is_leaf(self): | ||
| """Test checking if region is LEAF type.""" | ||
| leaf = Region(region_id=1, level=0, region_type=RegionType.LEAF) | ||
| composite = Region(region_id=2, level=1, region_type=RegionType.COMPOSITE) | ||
|
|
||
| assert leaf.get_type() == RegionType.LEAF | ||
| assert composite.get_type() != RegionType.LEAF | ||
| print("✓ Region LEAF type check") | ||
|
|
||
| def test_is_composite(self): | ||
| """Test checking if region is COMPOSITE type.""" | ||
| leaf = Region(region_id=1, level=0, region_type=RegionType.LEAF) | ||
| composite = Region(region_id=2, level=1, region_type=RegionType.COMPOSITE) | ||
|
|
||
| assert leaf.get_type() != RegionType.COMPOSITE | ||
| assert composite.get_type() == RegionType.COMPOSITE | ||
| print("✓ Region COMPOSITE type check") |
There was a problem hiding this comment.
These two tests can also be combined.
| def run_tests(): | ||
| """Run all Region tests.""" | ||
| print("=" * 70) | ||
| print("Region Class Test Suite") | ||
| print("=" * 70) | ||
|
|
||
| loader = unittest.TestLoader() | ||
| suite = unittest.TestSuite() | ||
| suite.addTests(loader.loadTestsFromTestCase(TestRegion)) | ||
|
|
||
| runner = unittest.TextTestRunner(verbosity=2) | ||
| result = runner.run(suite) | ||
|
|
||
| print("\n" + "=" * 70) | ||
| print("Test Summary") | ||
| print("=" * 70) | ||
| print(f"Tests run: {result.testsRun}") | ||
| print(f"Successes: {result.testsRun - len(result.failures) - len(result.errors)}") | ||
| print(f"Failures: {len(result.failures)}") | ||
| print(f"Errors: {len(result.errors)}") | ||
|
|
||
| if result.wasSuccessful(): | ||
| print("\n✓ All Region tests passed!") | ||
| return 0 | ||
| else: | ||
| print("\n✗ Some tests failed") | ||
| return 1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(run_tests()) |
There was a problem hiding this comment.
This boilerplate code can be removed too.
3 participants
What does this PR do?
Type of change: new feature
Overview: This PR integrates an automatical QDQ placment tool into ModelOpt.
This PR is the 1/4 parts of the change, it contains the following changes:
Part 1: #701
Part 2: #702
Part 3: #703
Part 4: #704
Usage
Testing
Implement unit tests, all tests could get passed.
Before your PR is "Ready for review"
Additional Information