-
Notifications
You must be signed in to change notification settings - Fork 64
Fixes #447: throw an error when printing output code in eager mode #528
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
Conversation
…ERPRET are both enabled
helion/runtime/settings.py
Outdated
| from typing import Protocol | ||
| from typing import Sequence | ||
| from typing import cast | ||
| from typing import cast, Literal, Protocol, Sequence, TYPE_CHECKING |
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.
please install and run pre-commit on your PR, our linter requires imports on different lines
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.
used pre-commit in my updated PR.
helion/runtime/settings.py
Outdated
| Check if eager mode is enabled before printing output code. If eager mode is enabled, raise an error. | ||
| """ | ||
| if self.ref_mode == RefMode.EAGER and self.print_output_code: | ||
| self.print_output_code = False |
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.
if we are throwing an exception, is this needed?
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.
deleted this line.
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.
please add a test, you can use mock.patch to deal with env vars or mock the settings
test/test_print_eager.py
Outdated
| torch.testing.assert_close(add(x, y), torch.add(x, y)) | ||
|
|
||
| def test_normal_code_print(self): | ||
| """Test that EagerCodePrintError is raised when using @helion.kernel with both settings""" |
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.
fix the comment here
|
Added unittest to check output print when ref_mode is either eager or normal. |
|
|
||
| x = torch.randn([512, 512], device="cuda", dtype=torch.float16) | ||
| y = torch.randn([512, 512], device="cuda", dtype=torch.float16) | ||
| torch.testing.assert_close(add(x, y), torch.add(x, y)) |
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.
with pytest.raises(exc.EagerCodePrintError):
should be over this one line only
|
|
||
| @helion.kernel( | ||
| use_default_config=True, | ||
| print_output_code=True, |
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.
The CI for ref mode passes the env variable to turn on ref mode, so you need to either mock the env var or disable the ref mode for this test
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.
thanks! some nits on the "ref eager mode" naming to make future lookups easier, otherwise it looks great!
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
Co-authored-by: Will Feng <yfeng.us@gmail.com>
…mode (pytorch#528) Co-authored-by: Will Feng <yfeng.us@gmail.com>
This pull request fixes #447: HELION_INTERPRET=1 silently hides HELION_PRINT_OUTPUT_CODE=1
When both HELION_INTERPRET and HELION_PRINT_OUTPUT_CODE are enabled, it will throw out an EagerCodePrintError.