[WIP] JIT Static Hooks: cpp tests #49547
Conversation
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit f83e028 (more details on the Dr. CI page):
Extra GitHub checks: 1 failed
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
[ghstack-poisoned]
ghstack-source-id: 3e0cd19354db0b5e527cc9d93461df3417b67aab Pull Request resolved: #49547
[ghstack-poisoned]
[ghstack-poisoned]
ghstack-source-id: 094f8e81ab00cb1ac2604d0c7c461ef52361e9d5 Pull Request resolved: #49547
[ghstack-poisoned]
[ghstack-poisoned]
ghstack-source-id: 83ff2498d910f7fd0e420fb53d244f5f1a16b23c Pull Request resolved: #49547
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
ghstack-source-id: 30771033af2697ced1c54e745fd0bedec39abc69 Pull Request resolved: #49547
[ghstack-poisoned]
Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
ghstack-source-id: 57b9b8c4f54d83ddd556cea62aed9422d04f7abb Pull Request resolved: #49547
.jenkins/pytorch/macos-test.sh
Outdated
| popd | ||
|
|
||
| # Run tests Python-side and export a script module. | ||
| # python test_jit_hooks.py -v |
There was a problem hiding this comment.
| # python test_jit_hooks.py -v |
test/jit_hooks/model.py
Outdated
| test_submodule_forward_single_input() | ||
| test_submodule_forward_multiple_inputs() | ||
| test_submodule_multiple_hooks_single_input() | ||
| test_submodule_multiple_hooks_multiple_inputs() | ||
| test_submodule_hook_return_nothing() | ||
| test_submodule_same_hook_repeated() | ||
|
|
||
| test_module_forward_single_input() | ||
| test_module_forward_multiple_inputs() | ||
| test_module_multiple_hooks_single_input() | ||
| test_module_multiple_hooks_multiple_inputs() | ||
| test_module_hook_return_nothing() | ||
| test_module_same_hook_repeated() | ||
|
|
||
| test_module_no_forward_input() | ||
| test_forward_tuple_input() |
There was a problem hiding this comment.
It seems that there's a one-to-one map from test case -> function that creates the model for that test case, and that all of the functions above are the same. You can probably create just one function that takes in a filename and model and then call that in a loop over a list of (test_name, test_model). Something like:
tests = [
("test_submodule_forward_single_input", test_submodule_forward_single_input()),
...
]
for name, model in tests:
m_scripted = torch.jit.script(model)
filename = ... # generated using name
torch.jit.save(model, filename)
There was a problem hiding this comment.
😮
that's some next level refactoring -- done ✅
test/jit_hooks/model.py
Outdated
|
|
||
| def test_submodule_hook_return_nothing(): | ||
| m_scripted = torch.jit.script(test_submodule_hook_return_nothing_model()) | ||
| m_scripted.save(save_name + "test_submodule_hook_return_nothing" + ".pt") |
There was a problem hiding this comment.
How come save is used here instead of torch.jit.save? Importing issues?
There was a problem hiding this comment.
no reason besides me not knowing the difference between the two
test/jit_hooks/test_jit_hooks.cpp
Outdated
|
|
||
| #include <iostream> | ||
|
|
||
| void test_submodule_multiple_hooks_single_input( |
There was a problem hiding this comment.
I think there is opportunity for refactoring here as well - a lot of the tests look the same. You could aggregate model paths, inputs, and outputs into some list or something and then run the same test code (load, run, compare output) for all (file, input, output) tuples. I think you should be able to deal with varying input/output types by expressing them as IValues and relying on IValue::equals for comparison.
test/jit_hooks/test_jit_hooks.cpp
Outdated
| std::ostringstream stream; | ||
| stream << output; | ||
| std::string output_str = stream.str(); | ||
| AT_ASSERT("([pre_hook_override_name, inner_mod_name], pre_hook_override2_fh1_fh2)" == output_str); |
There was a problem hiding this comment.
Could you use IValue comparison here instead by creating an ivalue::Tuple that contains the expected list and string?
test/jit_hooks/test_jit_hooks.cpp
Outdated
| std::ostringstream stream; | ||
| stream << output; | ||
| std::string output_str = stream.str(); | ||
| AT_ASSERT("([pre_hook_override_name2, outer_mod_name, inner_mod_name], pre_hook_override_fh1_fh2)" == output_str); |
test/jit_hooks/test_jit_hooks.cpp
Outdated
| std::cout << "----- module output: " << output << std::endl; | ||
| std::ostringstream stream; | ||
| stream << output; | ||
| std::string output_str = stream.str(); | ||
| AT_ASSERT("(11,)" == output_str);; |
test/jit_hooks/model.py
Outdated
| test_module_multiple_hooks_multiple_inputs() | ||
| test_module_hook_return_nothing() | ||
| test_module_same_hook_repeated() | ||
|
|
There was a problem hiding this comment.
Also, how come there are no C++ tests for direct forward invocation not invoking hooks?
There was a problem hiding this comment.
It's included in the cpp testing code already as 'test_module_forward_invocation_no_hooks_run', it's not in the .py file because it can re-use another test's saved model
There was a problem hiding this comment.
Ah, I see it now. I would also add to that a similar test to the one I commented on in the Python test PR: one where you access a submodule, store it in a temporary variable, and call that using Module::operator().
Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
ghstack-source-id: d13406b7cd3e2bc6096526f9e095b5c0cb9fd76e Pull Request resolved: #49547
Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
ghstack-source-id: 27d2df3c0416694b64d7ba20fe8d49e3041554e7 Pull Request resolved: #49547
Adds tests to verify that hooks are importable and runnable from CPP with the intended behavior. Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
ghstack-source-id: fc80cdeeb4810ada2051dd027b05e4329912eb2e Pull Request resolved: #49547
Adds tests to verify that hooks are importable and runnable from CPP with the intended behavior. Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
ghstack-source-id: 427f910327ae8746d9ba658597d816d2f5c1789c Pull Request resolved: #49547
Adds tests to verify that hooks are importable and runnable from CPP with the intended behavior. Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
ghstack-source-id: 867a1dc945bcc46fb81888778603270cdfef2c99 Pull Request resolved: #49547
Adds tests to verify that hooks are importable and runnable from CPP with the intended behavior. Differential Revision: [D25771118](https://our.internmc.facebook.com/intern/diff/D25771118) [ghstack-poisoned]
ghstack-source-id: 18fa05bdc2b2daed93a2987b1731d7448a08d5bc Pull Request resolved: #49547
Stack from ghstack:
Adds tests to verify that hooks are importable and runnable from CPP with the intended behavior.
Differential Revision: D25771118