Conversation
areusch
requested review from
comaniac,
jroesch,
junrushao,
merrymercy,
tqchen and
yzhliu
as code owners
areusch
force-pushed
the
parameterize-link-params-tests
branch
from
64639b9 to
8128067
Compare
Mousius
left a comment
Mousius
left a comment
There was a problem hiding this comment.
This is pretty cool, didn't realise we had this!
| linkable_dtype = tvm.testing.parameter( | ||
| *( | ||
| [f"uint{b}" for b in (8, 16, 32, 64)] | ||
| + [f"int{b}" for b in (8, 16, 32, 64)] | ||
| + ["float32", "float64"] | ||
| ) |
There was a problem hiding this comment.
I think you can do this a tiny bit neater?
| linkable_dtype = tvm.testing.parameter( | |
| *( | |
| [f"uint{b}" for b in (8, 16, 32, 64)] | |
| + [f"int{b}" for b in (8, 16, 32, 64)] | |
| + ["float32", "float64"] | |
| ) | |
| linkable_dtype = tvm.testing.parameter( | |
| *[f"uint{b}" for b in (8, 16, 32, 64)], | |
| *[f"int{b}" for b in (8, 16, 32, 64)], | |
| "float32", "float64" |
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Looks good to me, just had a couple small questions on it.
|
|
||
| # NOTE: Need to export_library() and load_library() to link all the Module(llvm, ...) | ||
| # against one another. | ||
| temp_dir = tempfile.mkdtemp() |
There was a problem hiding this comment.
This isn't related to the current PR, but do you know why tempfile.mkdtemp is used instead of tempfile.TemporaryDirectory? The mkdtemp requires manual cleanup which doesn't seem to be done here.
| else: | ||
| np.testing.assert_allclose(unlinked_output.numpy(), linked_output.numpy()) | ||
| def test_llvm_link_params(linkable_dtype): | ||
| ir_mod, param_init = _make_mod_and_params(linkable_dtype) |
There was a problem hiding this comment.
Now that linkable_dtype is a parameter, we can change _make_mod_and_params from a function to a fixture. This would let pytest make a distinction between setup errors in the module setup due to tvm.parser, and test errors in the linking.
There was a problem hiding this comment.
i'd like to address this one, but i'm a little short on time right now. hopefully we can merge this in the spirit of incremental progress and address later
There was a problem hiding this comment.
Sounds good, and agreed.
|
It has been a while since this PR was updated, @Lunderberg @Mousius @Lunderberg please leave a review or address the outstanding comments |
areusch
force-pushed
the
parameterize-link-params-tests
branch
from
8128067 to
f5a85c2
Compare
|
@Lunderberg I sync'd this one up, PTAL when you get a chance. |
areusch
force-pushed
the
parameterize-link-params-tests
branch
from
f5a85c2 to
f6d3494
Compare
| else: | ||
| np.testing.assert_allclose(unlinked_output.numpy(), linked_output.numpy()) | ||
| def test_llvm_link_params(linkable_dtype): | ||
| ir_mod, param_init = _make_mod_and_params(linkable_dtype) |
There was a problem hiding this comment.
Sounds good, and agreed.
4 participants
Cleaning up test_link_params to support the new parameterized testing.
cc @Lunderberg