-
Notifications
You must be signed in to change notification settings - Fork 86
issue/787 - Split run ops test logic and fix kwargs name in report #788
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
base: main
Are you sure you want to change the base?
Conversation
wooway777
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run.py的行为跟之前不一致了,需要确保主要行为完全一致
| operators_tested: int = 0 | ||
|
|
||
| @dataclass | ||
| class SingleTestResult: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
命名是不是没必要加这个Single
|
另外需要看一下,这些结果和信息打印的函数,能否跟现有的整合一下。 |
Sure, will compare the result with old run.py. |
This pr includes so much changes, I prefer to push this change and TestResult name change to next pr. |
wooway777
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
先改一轮吧。
以及我觉得,print、report、以及很多analyze和extract之类的方法,都是用来处理结果的。可以整合到一个类里面。
原则上讲,如果之前的定义不能满足现在的要求了,就去修改以前的东西,使得同样的信息可以从头用到尾。而不是写一个新的类似的东西把老的定义包起来。不然框架越写越大就不对劲了。
| import torch | ||
| import infinicore | ||
|
|
||
| from dataclasses import dataclass, field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
删?
| @@ -0,0 +1,180 @@ | |||
| # lib/printer.py | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
此行可删
| @@ -0,0 +1,52 @@ | |||
| from dataclasses import dataclass, field | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这东西肯定不叫types,大概structs好一点?
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
|
|
||
| # TODO: Rename it, current class name is abstract. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个todo是啥?
| operators_tested: int = 0 | ||
|
|
||
| @dataclass | ||
| class OperatorTestResult: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
目前这个设计还是很冗余。
有一个TestResult,然后还有一个OperatorTestResult,他俩信息重合了80%,多了两个方法;
然后这两个方法做的事情基本没区别。符号信息可以print的时候再加。
而且现在是以前的东西先反回一个result,然后再parse成新的result···
这两个result肯定可以整合一下。
| sys.exit(0) | ||
|
|
||
| # 2. Preparation | ||
| dirver = TestDriver() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
| @@ -0,0 +1,180 @@ | |||
| # lib/printer.py | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
文件可以就叫summary.py
loader printer reporter多了就不专业了
现在的主要问题是,设计不合理合不了 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the InfiniCore operator test runner by splitting monolithic test logic into modular framework components and fixes kwargs name handling in the test reporter.
Key Changes:
- Extracted test discovery, execution, and reporting logic into separate framework modules (
loader.py,driver.py,printer.py) - Moved
TestResultclass fromentities.pyto newtypes.pymodule for better organization - Enhanced kwargs name handling in
reporter.pyto properly display input/output tensor names in test reports
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| test/infinicore/run.py | Refactored from 567 lines to 194 lines by delegating logic to framework components; now acts as a thin orchestration layer |
| test/infinicore/framework/types.py | New file defining core data structures: TestResult, TestTiming, and OperatorTestResult |
| test/infinicore/framework/loader.py | New TestDiscoverer class handling operator test file discovery and validation |
| test/infinicore/framework/driver.py | New TestDriver class managing test execution, output capture, and result aggregation |
| test/infinicore/framework/printer.py | New ConsolePrinter class handling all console output formatting and summaries |
| test/infinicore/framework/reporter.py | Enhanced kwargs handling to properly resolve and display tensor names instead of null/index values |
| test/infinicore/framework/entities.py | Removed TestResult class (moved to types.py) |
| test/infinicore/framework/base.py | Updated import to use TestResult from types module |
| test/infinicore/framework/init.py | Added exports for new types and classes |
| test/infinicore/framework/datatypes.py | Minor formatting change (added import statement) |
Comments suppressed due to low confidence (1)
test/infinicore/run.py:145
- This assignment to 'target_ops' is unnecessary as it is redefined before this value is used.
target_ops = valid_ops
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._print_op_list("⏭️ SKIPPED OPERATORS", skipped) | ||
|
|
||
| # PARTIAL | ||
| if partial: | ||
| self._print_op_list("⚠️ PARTIAL IMPLEMENTATIONS", partial) |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation: This line has an extra space before 'self._print_op_list', making it have 13 spaces instead of 12. This should match the indentation of the surrounding lines for consistency.
| self._print_op_list("⏭️ SKIPPED OPERATORS", skipped) | |
| # PARTIAL | |
| if partial: | |
| self._print_op_list("⚠️ PARTIAL IMPLEMENTATIONS", partial) | |
| self._print_op_list("⏭️ SKIPPED OPERATORS", skipped) | |
| # PARTIAL | |
| if partial: | |
| self._print_op_list("⚠️ PARTIAL IMPLEMENTATIONS", partial) |
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
|
|
||
| # TODO: Rename it, current class name is abstract. |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment should not be left in production code. Either rename the class to something more specific (e.g., 'OperatorTestResultDetail', 'TestCaseResult', or 'SingleTestResult') or remove the TODO if the current name is acceptable.
| # TODO: Rename it, current class name is abstract. |
| bench_mode = args.bench if args.bench != "both" else "both" | ||
| print(f"Benchmark mode: {bench_mode.upper()} timing") | ||
|
|
||
| target_ops = None |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable target_ops is initialized here but not used before being potentially reassigned in the conditional blocks below. Consider initializing it only once before the conditional logic or removing this initialization since it's assigned in all branches.
| target_ops = valid_ops | ||
|
|
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate assignment to target_ops. This line is redundant as target_ops is already assigned on line 145 within the else block. The variable is being reassigned to the same value immediately after.
| target_ops = valid_ops |
| from dataclasses import dataclass, field | ||
|
|
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'dataclass' is not used.
Import of 'field' is not used.
| from dataclasses import dataclass, field |
| import importlib.util | ||
| from io import StringIO | ||
| from contextlib import contextmanager | ||
| from .types import OperatorTestResult, TestTiming |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'TestTiming' is not used.
| from .types import OperatorTestResult, TestTiming | |
| from .types import OperatorTestResult |
| @@ -0,0 +1,180 @@ | |||
| # lib/printer.py | |||
| import sys | |||
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'sys' is not used.
| import sys |
| @@ -0,0 +1,180 @@ | |||
| # lib/printer.py | |||
| import sys | |||
| from .types import OperatorTestResult, TestTiming | |||
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'TestTiming' is not used.
Import of 'OperatorTestResult' is not used.
| return "infinicore" in content and ( | ||
| "BaseOperatorTest" in content or "GenericTestRunner" in content | ||
| ) | ||
| except: |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except block directly handles BaseException.
| except: | |
| except Exception: |
Revision 2
Description
func behavior fix and class rename
Test evidence
We comment torch_operator method to create a skipped test case, in this way we have a success case, a partial case and a skipped case.
Command:
Output:
Help info
Description
Split run ops test logic and fix kwargs name in report
Test evidence
json string in test report