Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 80 additions & 7 deletions modelopt/onnx/quantization/ort_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@
import glob
import io
import os
import pathlib
import platform
import re
import shutil
import subprocess # nosec B404
import sys
from collections.abc import Sequence
from contextlib import redirect_stderr, redirect_stdout
from importlib.metadata import PackageNotFoundError, distribution

import onnxruntime as ort
from onnxruntime.quantization.operators.qdq_base_operator import QDQOperatorBase
Expand Down Expand Up @@ -126,6 +128,78 @@ def _check_for_tensorrt(min_version: str = "10.0"):
)


def _find_cudnn_bin_dir():
"""Locate the nvidia cudnn bin directory inside site-packages."""
for pkg_name in ("nvidia-cudnn-cu12", "nvidia-cudnn-cu13"):
try:
dist = distribution(pkg_name)
except PackageNotFoundError:
continue
for f in dist.files or []:
if f.name.startswith("cudnn64_") and f.name.endswith(".dll"):
bin_dir = str(pathlib.Path(f.locate()).parent)
if os.path.isdir(bin_dir):
return bin_dir
return None


def _load_extra_cudnn_dlls():
"""Load any cuDNN DLLs from site-packages that ORT's preload_dlls() missed.

TEMPORARY WORKAROUND: This function exists because ort.preload_dlls() has a
hardcoded list of cuDNN sub-libraries which may be incomplete for newer cuDNN
versions (e.g. cuDNN 9.21 added cudnn_engines_tensor_ir64_9.dll, cuDNN 9.20
added cudnn_cnn64_9.dll). Once ort.preload_dlls() is fixed upstream to
dynamically discover all cuDNN DLLs, this function and its helper
(_find_cudnn_bin_dir) should be removed.

This scans the nvidia-cudnn bin directory and loads any cudnn*.dll not already
loaded in the process.
"""
import ctypes
import ctypes.wintypes

cudnn_bin_dir = _find_cudnn_bin_dir()
if not cudnn_bin_dir:
logger.debug(
"nvidia-cudnn bin directory not found in site-packages, skipping extra DLL load"
)
return

dll_files = sorted(glob.glob(os.path.join(cudnn_bin_dir, "cudnn*.dll")))
if not dll_files:
logger.debug("No cudnn*.dll files found in %s", cudnn_bin_dir)
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you add a logger.debug statement that no cudnn dll files were found?


get_module_handle_w = ctypes.windll.kernel32.GetModuleHandleW # type: ignore[attr-defined]
get_module_handle_w.argtypes = [ctypes.wintypes.LPCWSTR]
get_module_handle_w.restype = ctypes.wintypes.HMODULE

loaded = []
skipped = []
failed = []
for dll_path in dll_files:
dll_name = os.path.basename(dll_path)
if get_module_handle_w(dll_name):
skipped.append(dll_name)
continue
try:
ctypes.CDLL(dll_path)
loaded.append(dll_name)
except OSError as e:
failed.append(dll_name)
logger.warning(f"Failed to load {dll_name} from site-packages: {e}")

if skipped:
logger.debug(f"Already loaded (skipped): {skipped}")
if loaded:
logger.info(
f"Loaded {len(loaded)} extra cuDNN DLLs that ort.preload_dlls() missed: {loaded}"
)
if failed:
logger.warning(f"Failed to load {len(failed)} cuDNN DLLs: {failed}")

Comment on lines +146 to +201

@coderabbitai coderabbitai Bot Apr 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Preload failure can still return success (True).

If ort.preload_dlls() raises, the exception is only logged and the flow can still hit the success branch and return True. Also, extra-DLL load failures are not propagated to the caller, so _check_for_libcudnn() may report success while cuDNN is still partially unavailable.

Proposed fix
-def _load_extra_cudnn_dlls():
+def _load_extra_cudnn_dlls() -> list[str]:
@@
-        return
+        return []
@@
-        return
+        return []
@@
     if failed:
         logger.warning(f"Failed to load {len(failed)} cuDNN DLLs: {failed}")
+    return failed
@@
-            try:
+            preload_exception: Exception | None = None
+            try:
                 with redirect_stdout(captured), redirect_stderr(captured):
                     ort.preload_dlls()
             except Exception as e:
+                preload_exception = e
                 logger.warning(f"onnxruntime.preload_dlls() raised an exception: {e}")
@@
-            if f"Failed to load {core_cudnn_dll}" in preload_output:
+            preload_failed = preload_exception is not None or (
+                f"Failed to load {core_cudnn_dll}" in preload_output
+            )
+            extra_failed: list[str] = []
+            if not preload_failed and platform.system() == "Windows":
+                extra_failed = _load_extra_cudnn_dlls()
+                preload_failed = len(extra_failed) > 0
+
+            if preload_failed:
                 logger.error(
                     f"onnxruntime.preload_dlls() was called but {core_cudnn_dll} failed to load. "
                     "cuDNN DLLs were NOT successfully loaded from site-packages."
                 )
             else:
-                if platform.system() == "Windows":
-                    _load_extra_cudnn_dlls()
                 logger.info(
                     "onnxruntime.preload_dlls() succeeded — CUDA/cuDNN DLLs loaded"
                     " from site-packages. Verify version compatibility at"
                     " https://onnxruntime.ai/docs/execution-providers/CUDA-ExecutionProvider.html#requirements."
                 )
                 return True

Also applies to: 229-252

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modelopt/onnx/quantization/ort_utils.py` around lines 147 - 201, The current
logic in _load_extra_cudnn_dlls (and the caller _check_for_libcudnn) swallows
exceptions from ort.preload_dlls() and treats partial extra-DLL load failures as
success; change the flow so that if ort.preload_dlls() raises we log and
immediately return False (or re-raise), and after attempting to load extra cudnn
DLLs, if any DLLs failed to load (the failed list is non-empty) return False (or
raise an appropriate error) instead of allowing a success return; update callers
of _load_extra_cudnn_dlls/_check_for_libcudnn to respect the boolean failure and
propagate failure to the caller so that partial/failed cuDNN loads are not
reported as available.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai This seems like a non issue, since there is no real way to log failure through ort_preload_dlls. So we just check if the function loads cudnn64 main dll and if it is not loaded then only log failure. The extra dll will try to load all dlls in venv nvidia folder . There should be no error logging there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hthadicherla, that's a fair clarification — the best-effort philosophy for extra DLL loading makes sense, and I agree that failures in _load_extra_cudnn_dlls() should remain warnings rather than propagating as hard errors.

One narrow concern still stands though: if ort.preload_dlls() raises an exception, captured is left empty, so the "Failed to load cudnn64_9" check silently passes (empty string never contains the pattern), _load_extra_cudnn_dlls() is called, and the function returns True — even though the main preload attempt actually failed with an exception. The exception is already being caught and logged as a warning, so this isn't a silent failure, but the function still surfaces as "success" to its caller.

A minimal guard for just that case (without changing the best-effort DLL logic at all):

preload_exception = None
try:
    with redirect_stdout(captured), redirect_stderr(captured):
        ort.preload_dlls()
except Exception as e:
    preload_exception = e
    logger.warning(f"onnxruntime.preload_dlls() raised an exception: {e}")

preload_output = captured.getvalue()
if preload_output:
    logger.warning(f"preload_dlls() output:\n{preload_output}")

core_cudnn_dll = "cudnn64_9" if platform.system() == "Windows" else "libcudnn_adv"
if preload_exception is not None or f"Failed to load {core_cudnn_dll}" in preload_output:
    logger.error(...)
else:
    if platform.system() == "Windows":
        _load_extra_cudnn_dlls()  # best-effort, failures stay as warnings
    logger.info("onnxruntime.preload_dlls() succeeded ...")
    return True

That keeps _load_extra_cudnn_dlls() fully best-effort while correctly treating a raised exception as a load failure. Happy to drop this entirely if you consider the exception path acceptable in practice.


🧠 Learnings used
Learnt from: kevalmorabia97
Repo: NVIDIA/Model-Optimizer PR: 974
File: modelopt/torch/puzzletron/pruning/pruning_utils.py:195-201
Timestamp: 2026-03-06T14:26:21.439Z
Learning: In NVIDIA/Model-Optimizer, for PyTorch >= 2.6, torch.load() calls without an explicit weights_only argument are safe. Do not flag bare torch.load(...) as a security issue in files under the modelopt package (e.g., modelopt/torch/puzzletron/pruning/pruning_utils.py) as long as the PyTorch version constraint is maintained. If supporting PyTorch < 2.6, require an explicit weights_only argument to torch.load() to avoid potential issues.


def _check_for_libcudnn():
# TODO: handle multiple calls to this function
logger.info("Checking for cuDNN library")
Expand All @@ -150,10 +224,6 @@ def _check_for_libcudnn():
f"cuDNN not found in {env_variable}. "
"Attempting onnxruntime.preload_dlls() to load from site-packages..."
)
# preload_dlls() does not raise on failure — it silently prints
# "Failed to load ..." messages. Capture its output and check
# whether the key cuDNN DLL actually loaded.
cudnn_dll = "cudnn" if platform.system() == "Windows" else "libcudnn_adv"
captured = io.StringIO()
try:
with redirect_stdout(captured), redirect_stderr(captured):
Expand All @@ -163,14 +233,17 @@ def _check_for_libcudnn():

preload_output = captured.getvalue()
if preload_output:
logger.debug(f"preload_dlls() output:\n{preload_output}")
logger.warning(f"preload_dlls() output:\n{preload_output}")

if f"Failed to load {cudnn_dll}" in preload_output:
core_cudnn_dll = "cudnn64_9" if platform.system() == "Windows" else "libcudnn_adv"
if f"Failed to load {core_cudnn_dll}" in preload_output:
logger.error(
f"onnxruntime.preload_dlls() was called but {cudnn_dll} failed to load. "
f"onnxruntime.preload_dlls() was called but {core_cudnn_dll} failed to load. "
"cuDNN DLLs were NOT successfully loaded from site-packages."
)
else:
if platform.system() == "Windows":
_load_extra_cudnn_dlls()
logger.info(
"onnxruntime.preload_dlls() succeeded — CUDA/cuDNN DLLs loaded"
" from site-packages. Verify version compatibility at"
Expand Down
Loading