Add tests for Float8Tensor at graph boundaries #196
Conversation
facebook-github-bot
added
the
CLA Signed
|
@drisspg has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
test/test_compile.py
Outdated
| x = torch.randn(16, 16, device="cuda") | ||
| y_compiled = compiled_mod(x) | ||
|
|
||
| assert not isinstance( |
There was a problem hiding this comment.
lets call flatten so that this test doens't become stale
There was a problem hiding this comment.
rip this assert haha. I promised Voz I would add this check more natively in dynamo here: pytorch/pytorch#118211, so eventually you (hopefully) won't need to feel so paranoid
|
@drisspg has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
test/test_compile.py
Outdated
| @@ -76,5 +78,79 @@ def test_inductor(fullgraph, emulate: bool, linear_type: bool, dtype: torch.dtyp | |||
| _test_compile_base("inductor", fullgraph, emulate, linear_type, dtype) | |||
|
|
|||
|
|
|||
| class TestGraphBreaks: | |||
There was a problem hiding this comment.
(1) test_compile is a weird file when we have test/dynamo/*
(2) Can we use regular dynamo testing infra? compile counter, etc?
(3) 1 test class per test file please
(4) This class name is wrong? It's testing fp8 + dynamo, not testing graph breaks (I would imagine TestGraphBreaks would test things like deep dynamo internal working)
There was a problem hiding this comment.
1.) We don't have test/dynamo/*
2.) Sure is there a pointer for this?
3.) I hadn't heard of this rule before is this in some style guide convention somewhere?
4.) I thought thats implied by being in Float8Experimetnal, no?
There was a problem hiding this comment.
(1) I know, but you can put a test there that depends on this, and have CI install it, right? Alternatively, if you depend on dynamo, just pull the deps out
(2) Yes! Check any test under test/dynamo/*, you dont have to inherit from the base class, but it does cover things like .reset() for you nicely (which I think we missed here!) - grep for CompileCounter :)
(3) No, but I think it helps with organization. Its just a tiny nipick
(4) True, that is my mistake.
test/test_compile.py
Outdated
|
|
||
| mod = self.MockLinear(graph_break=False).cuda() | ||
| x = torch.randn(2, 2, device="cuda") | ||
| compiled_to_float = torch.compile(to_float) |
There was a problem hiding this comment.
to_float could hit a skip, and look exactly identical, and still return as compiled_to_float, and this test would pass.
There was a problem hiding this comment.
you are right thats a bug 120 should be removed
There was a problem hiding this comment.
Sorry, thats not what I mean! I mean that if you dont use our test infra that counts frames, you could hit a skip, get a frame_count of 0, and this test would pass!
There was a problem hiding this comment.
Discussed offline - this is the only grounds for my reject.
|
@drisspg has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@voznesenskym Updated to use CompileCounterWithBackend |
Summary
This is a copy with some tweaks of: #166