Add compatibility checks for C++ extensions #2467
Conversation
| @@ -1,6 +1,10 @@ | |||
| _HAS_OPS = False | |||
There was a problem hiding this comment.
This can probably be removed now
| except (ImportError, OSError): | ||
| pass | ||
|
|
||
|
|
||
| def _assert_has_ops(): |
There was a problem hiding this comment.
I should do something similar for the video and image ops, but I'll do that in a follow-up PR once we define more precisely if this implementation is enough
| @@ -23,10 +27,25 @@ def _register_extensions(): | |||
| try: | |||
| _register_extensions() | |||
| _HAS_OPS = True | |||
| def _has_ops(): | |||
| return True | |||
| except (ImportError, OSError): | |||
| pass | |||
There was a problem hiding this comment.
Thanks for addressing this!
Perhaps it's possible to define _has_ops function here that returns (bool has_ops, str err_msg). Then the exact error message from the exception can be used in _assert_has_ops
There was a problem hiding this comment.
Thanks for the review, I forgot to answer this.
I think this should be possible to be done, let me try this.
Given that torchscript doesn't support globals, I think the only way we could have this is to have pre-canned error messages that are used to construct the _has_ops function.
Something like
try:
_register_extensions()
...
except (ImportError, OSError) as err:
err_str = repr(err)
if err_type_1 in err_str:
def _has_ops():
...
elif err_type_2 in err_str:
...which makes it a bit less readable, and kinds of defeats the purpose of having the error message being returned because we would only be able to nicely print a few types of errors (the ones we hard-coded).
But maybe I'm missing something here?
|
Since torchvision is using TORCH_LIBRARY we should probably also bring this to attention of @jamesr66a for torchbind. |
Codecov Report
@@ Coverage Diff @@
## master #2467 +/- ##
==========================================
+ Coverage 73.12% 73.14% +0.02%
==========================================
Files 96 96
Lines 8316 8335 +19
Branches 1293 1294 +1
==========================================
+ Hits 6081 6097 +16
- Misses 1838 1840 +2
- Partials 397 398 +1
Continue to review full report at Codecov.
|
* Add compatibility checks for C++ extensions * Fix lint
* Add compatibility checks for C++ extensions * Fix lint
Fixes #2148
For now, I don't print the PyTorch and torchvision in the error messages as it would require a bit more work (the torchvision version is a global variable if just imported from
.version), but I believe it's already an improvement compared to what we had before.Another thing we might want to consider doing is to add the original stack trace in the error message, but again if we want to make it work with torchscript we would probably need to wrap this on a function that returns the string.
@eellison do you see any issue with the current approach of having the
_has_opsimplementation be switched at runtime?