Skip to content
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

Clarify error message if onnx_graphsurgeon is not installed? #353

Closed
mattpopovich opened this issue Mar 9, 2022 · 8 comments · Fixed by #366
Closed

Clarify error message if onnx_graphsurgeon is not installed? #353

mattpopovich opened this issue Mar 9, 2022 · 8 comments · Fixed by #366
Labels
code quality Code format and unit tests dependencies Pull requests that update a dependency file help wanted Extra attention is needed

Comments

@mattpopovich
Copy link
Contributor

mattpopovich commented Mar 9, 2022

🐛 Describe the bug

When following the TensorRT deployment instructions, there is a point where we try to use YOLOTRTGraphSurgeon, but to use that, we need to import onnx_graphsurgeon as gs. Otherwise, this line will fail because gs = None.

Traceback (most recent call last):
  File "tensorrt_test.py", line 57, in <module>
    export_tensorrt_engine(
  File "/home/username/git/yolov5-rt-stack/yolort/runtime/trt_helper.py", line 69, in export_tensorrt_engine
    yolo_gs = YOLOTRTGraphSurgeon(checkpoint_path, version=version, input_sample=input_sample)
  File "/home/username/git/yolov5-rt-stack/yolort/relay/trt_graphsurgeon.py", line 76, in __init__
    self.graph = gs.import_onnx(onnx.load(onnx_model_path))
AttributeError: 'NoneType' object has no attribute 'import_onnx'

I'm not sure why you catch the ImportError if onnx_graphsurgeon is not found as YOLOTRTGraphSurgeon is the only thing that is in that file and it seems like we need onnx_graphsurgeon, but just wanted to bring this to your attention.

Low priority 😄

Versions

root@user# python3 collect_env.py 
Collecting environment information...
PyTorch version: 1.11.0a0+17540c5
Is debug build: False
CUDA used to build PyTorch: 11.6
ROCM used to build PyTorch: N/A

OS: Ubuntu 20.04.3 LTS (x86_64)
GCC version: (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Clang version: Could not collect
CMake version: version 3.21.3
Libc version: glibc-2.31

Python version: 3.8.12 | packaged by conda-forge | (default, Oct 12 2021, 21:59:51)  [GCC 9.4.0] (64-bit runtime)
Python platform: Linux-5.4.0-100-generic-x86_64-with-glibc2.10
Is CUDA available: True
CUDA runtime version: 11.6.55
GPU models and configuration: 
GPU 0: NVIDIA GeForce GTX 1080
GPU 1: NVIDIA GeForce GTX 1080
GPU 2: NVIDIA GeForce GTX 1080

Nvidia driver version: 470.103.01
cuDNN version: Probably one of the following:
/usr/lib/x86_64-linux-gnu/libcudnn.so.8.3.2
/usr/lib/x86_64-linux-gnu/libcudnn_adv_infer.so.8.3.2
/usr/lib/x86_64-linux-gnu/libcudnn_adv_train.so.8.3.2
/usr/lib/x86_64-linux-gnu/libcudnn_cnn_infer.so.8.3.2
/usr/lib/x86_64-linux-gnu/libcudnn_cnn_train.so.8.3.2
/usr/lib/x86_64-linux-gnu/libcudnn_ops_infer.so.8.3.2
/usr/lib/x86_64-linux-gnu/libcudnn_ops_train.so.8.3.2
HIP runtime version: N/A
MIOpen runtime version: N/A
Is XNNPACK available: True

Versions of relevant libraries:
[pip3] mypy-extensions==0.4.3
[pip3] numpy==1.22.2
[pip3] pytorch-lightning==1.5.10
[pip3] pytorch-quantization==2.1.2
[pip3] torch==1.11.0a0+17540c5
[pip3] torch-tensorrt==1.1.0a0
[pip3] torchmetrics==0.7.2
[pip3] torchtext==0.12.0a0
[pip3] torchvision==0.12.0a0
[conda] magma-cuda110             2.5.2                         5    local
[conda] mkl                       2019.5                      281    conda-forge
[conda] mkl-include               2019.5                      281    conda-forge
[conda] mypy_extensions           0.4.3            py38h578d9bd_4    conda-forge
[conda] numpy                     1.22.2           py38h6ae9a64_0    conda-forge
[conda] pytorch-lightning         1.5.10                   pypi_0    pypi
[conda] pytorch-quantization      2.1.2                    pypi_0    pypi
[conda] torch                     1.11.0a0+17540c5          pypi_0    pypi
[conda] torch-tensorrt            1.1.0a0                  pypi_0    pypi
[conda] torchmetrics              0.7.2                    pypi_0    pypi
[conda] torchtext                 0.12.0a0                 pypi_0    pypi
[conda] torchvision               0.12.0a0                 pypi_0    pypi
@zhiqwang
Copy link
Owner

zhiqwang commented Mar 9, 2022

Hi @mattpopovich ,

The reason is that we want to make onnx_graphsurgeon optional for other libraries (if the user don't want to use TensorRT), and a good way is to add some assertion information to tell the user to install onnx_graphsurgeon when calling gs.

https://github.com/zhiqwang/yolov5-rt-stack/blob/0345ed6171f0e6c20b465fc082cbd3b8aa0bdf07/yolort/relay/trt_graphsurgeon.py#L72

@mattpopovich
Copy link
Contributor Author

Got it, I'll make a PR in a bit to add some assertion information!

@zhiqwang zhiqwang added the code quality Code format and unit tests label Mar 10, 2022
@zhiqwang
Copy link
Owner

zhiqwang commented Mar 12, 2022

Hi @mattpopovich ,

As you have found before, there are some similar issues elsewhere. I just released version 0.6.2, and I also introduce this poor implementation on this new releases for rapid development.

https://github.com/zhiqwang/yolov5-rt-stack/blob/d088d5fca243dd0f04e0d69b9565bf28d41aaf03/yolort/data/data_module.py#L9-L12

The main reason for these things is that I want to make the core functionality of yolort as light as possible. Since there are many similar problems in yolort, I think it's time to resolve this problem in the next release. TorchAudio implement a is_module_available() function to handle such scenarios:

def is_module_available(*modules: str) -> bool:
    r"""Returns if a top-level module with :attr:`name` exists *without**
    importing it. This is generally safer than try-catch block around a
    `import X`. It avoids third party libraries breaking assumptions of some of
    our tests, e.g., setting multiprocessing start method when imported
    (see librosa/#747, torchvision/#544).
    """
    return all(importlib.util.find_spec(m) is not None for m in modules)

And then they implement a decorator requires_kaldi() to assert the existence of kaldi as an example. Maybe we could follow this approach. As such we could implement something like requires_tensorrt(), requires_onnxruntime(), requires_onnxgraphsurgeon(), requires_lightning() and so on. This mechanism is a litter complex, but I set these here as an implementation option, and let me know if you have more concerns.

from functools import wraps

def is_tensorrt_available():
    return is_module_available("tensorrt")

def requires_tensorrt():
    if is_tensorrt_available():
        def decorator(func):
            return func
    else:
        def decorator(func):
            @wraps(func)
            def wrapped(*args, **kwargs):
                raise RuntimeError(f"{func.__module__}.{func.__name__} requires TensorRT")
            return wrapped
    return decorator

More context

@zhiqwang zhiqwang added help wanted Extra attention is needed dependencies Pull requests that update a dependency file labels Mar 12, 2022
@mattpopovich
Copy link
Contributor Author

Would you prefer to write specific requires_tensorrt(), requires_onnxruntime(), etc. functions or should we just use one requires_module() to then do requires_module('tensorrt'), requires_module('onnxruntime'), etc.?

I'm leaning towards just using requires_module().

I feel like they made specific is_modulename_available() such as is_kaldi_available() because doing just requires_module('torchaudio._torchaudio') doesn't include any hint to kaldi as was discussed here in reference to sox. And the requires_kaldi() function seems to be implemented in order to have a more descriptive error message (Runtime Error: ... requires kaldi).

@zhiqwang
Copy link
Owner

zhiqwang commented Mar 15, 2022

Hi @mattpopovich

I feel like they made specific is_modulename_available() such as is_kaldi_available() because doing just requires_module('torchaudio._torchaudio') doesn't include any hint to kaldi as was discussed pytorch/audio#692 (comment) in reference to sox.

I agree with you here, and this approach is also really a bit complicated and not necessary for us. Seems the dependencies for kaldi in TorchAudio are generated from C++, we still only have the python level dependencies for now.

Maybe the _module_available method in lightning-flash is more suitable for us? (User case of _module_available)

def _module_available(module_path: str) -> bool:
    """
    Check if a path is available in your environment.
    >>> _module_available('os')
    True
    >>> _module_available('bla.bla')
    False
    """
    try:
        return find_spec(module_path) is not None
    except AttributeError:
        # Python 3.6
        return False
    except ModuleNotFoundError:
        # Python 3.7+
        return False
    except ValueError:
        # Sometimes __spec__ can be None and gives a ValueError
        return True

And then they use a example_requires mechanism to verify the existence of determined package. (User case of example_requires)

def example_requires(module_paths: Union[str, List[str]]):
    return requires(module_paths)(lambda: None)()

@mattpopovich
Copy link
Contributor Author

mattpopovich commented Mar 15, 2022

Check out this commit that I made. It follows the same thing that pytorch/audio did and I think is pretty straightforward.

root@user:/home/user/git/yolov5-rt-stack/# python3 function_using_YOLOTRTGraphSurgeon.py
We're using TensorRT: 8.2.3.0 on cuda device: 0.
NOTE! Installing ujson may make loading annotations faster.

[...]

Traceback (most recent call last):
  File "function_using_YOLOTRTGraphSurgeon.py", line 63, in <module>
    export_tensorrt_engine(
  File "/home/user/git/yolov5-rt-stack/yolort/runtime/trt_helper.py", line 69, in export_tensorrt_engine
    yolo_gs = YOLOTRTGraphSurgeon(checkpoint_path, version=version, input_sample=input_sample)
  File "/home/user/git/yolov5-rt-stack/yolort/utils/module_utils.py", line 39, in wrapped
    raise RuntimeError(f"{func.__module__}.{func.__name__} requires {req}")
RuntimeError: yolort.relay.trt_graphsurgeon.YOLOTRTGraphSurgeon requires module: onnx_graphsurgeon
root@user:/home/user/git/yolov5-rt-stack/# pip install onnx_graphsurgeon

[successful]

root@user:/home/user/git/yolov5-rt-stack/# python3 function_using_YOLOTRTGraphSurgeon.py

[no Traceback]

If you're okay with this change, I'll do the same for TensorRT, onnxruntime, and lightning. Then I'll make a PR to close this issue.

Additionally, let me know if you are okay with the placement of yolort/utils/module_utils.py or if you would like those functions moved elsewhere.

@zhiqwang
Copy link
Owner

Hi @mattpopovich

If you're okay with this change, I'll do the same for TensorRT, onnxruntime, and lightning. Then I'll make a PR to close this issue.

Thanks, I think it's very pretty!

Additionally, let me know if you are okay with the placement of yolort/utils/module_utils.py or if you would like those functions moved elsewhere.

And there is already a file aimed to resolve the version and dependencies problem: https://github.com/zhiqwang/yolov5-rt-stack/blob/main/yolort/utils/dependency.py . I guess it will be better if we could move the yolort/utils/module_utils.py to yolort/utils/dependency.py.

@mattpopovich
Copy link
Contributor Author

And there is already a file aimed to resolve the version and dependencies problem: https://github.com/zhiqwang/yolov5-rt-stack/blob/main/yolort/utils/dependency.py . I guess it will be better if we could move the yolort/utils/module_utils.py to yolort/utils/dependency.py.

Sounds good. I'll make the PR tomorrow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Code format and unit tests dependencies Pull requests that update a dependency file help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants