-
Notifications
You must be signed in to change notification settings - Fork 7k
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
make transforms v2 JIT scriptable #7135
Conversation
Test failures for |
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 Philip, I left some minor comments but this looks great
@@ -642,6 +649,74 @@ def test_call_consistency(config, args_kwargs): | |||
) | |||
|
|||
|
|||
@consistency_configs |
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.
Q: Is it necessary to test JIT support across all consistency configs? I fear that we're adding a lot of test time for little more coverage (but my estimation of the test time could be way off, as well as that of the coverage).
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.
On my system we are talking about roughly 20 seconds:
pytest test/test_prototype_transforms_consistency.py::test_jit_consistency 21,64s user 1,76s system 185% cpu 12,616 total
I don't think that we are in a realm where we have to worry about CI time yet. Keep in mind, these are not our v2 functional tests, but rather just the consistency ones. In total we only run 143 tests here, which by default check 8 images. So we are at roughly 1k images that we are testing.
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 a lot Philip, one minor Q below but LGTM when green!
That's a very nice way to remove one of the major roadblocks for migration out of the prototype area
Reviewed By: vmoens Differential Revision: D43116123 fbshipit-source-id: 42719d4dae7c7637bc2411b63c375659e59149df
This is a very early draft for closing #6711. It is based on the idea in #6711 (comment) and requires only very minimal configuration.
ToDO:
cc @vfdev-5 @bjuncek