mirror of
https://github.com/NVIDIA/Model-Optimizer.git
synced 2026-11-03 08:14:29 +08:00
gcunhaseandmodelopt-fix-agent-bot
5e2b0947dc
Fixes the 11 CodeRabbit findings that survived triage on PR #2240. Two Security/Major items (public-API artifact size guards on :func:`score`; removing target names from logs) were consciously deferred per author call. Documentation ============= * :pr:`comment:3883483129` -- ``docs/.../_onnx_quantization.rst``: ``[expr for i, ex in enumerate(ds) if i < 500]`` doesn't stop the streaming iterator -- Python evaluates every element of ``ds`` against the ``i < 500`` filter, so users copy-pasting the doc drain the full 50k-image ImageNet-1k validation split each time. Replaced with ``itertools.islice(ds, 500)``. * :pr:`comment:3883483137` -- picker examples referenced ``scores`` but the preceding block only defined ``result``; copying the block raised ``NameError``. Added ``scores = result["scores"]`` bridging line. * :pr:`comment:3883483146` -- CLI ``--metric`` help said "activations", but :func:`score` runs both graphs through ORT and compares **graph outputs**. Same imprecision propagated into the ``Metric`` enum docstring and the ``metrics.py`` module docstring. Standardised on "graph outputs" across all three. * :pr:`comment:3883483177` -- ``op_types_scope`` docstring still said unsupported targets are scored ``0.0``; the earlier ``failed``-field rework moved them to a distinct list. Doc now documents the new behavior and the "0.0 means free / ``failed`` means we don't know" distinction. Correctness =========== * :pr:`comment:3883483142` -- ``_render_ranked_table`` printed "no quantizable targets found" whenever ``scores`` was empty, hiding the case where every probe failed. Split into three branches: empty + no failed = "no targets"; empty + failed = "no scores produced; N target(s) failed"; non-empty + failed adds a trailing "N target(s) failed to probe" line. * :pr:`comment:3883483161` -- ``cos_dist`` returned ``1.0`` when both reference and quantized outputs were all-zero (``0 / (0 + _EPS) = 0`` -> distance 1). A probe whose outputs are both zero should score as identical, not maximally sensitive. Guarded with ``np.where(norm > 0, ..., 1.0)`` so ``distance == 0.0``. Added ``test_both_zero_vectors_return_zero_distance`` as a regression witness in ``test_metrics.py``. * :pr:`comment:3883483169` -- :func:`suggest_exclusion` coverage-mode docstring said "largest target set whose cumulative score stays at or below ``coverage * total_mass``", but the implementation is a rank-prefix walker that stops at the first non-fitting target (e.g. ``{a: 4, b: 3, c: 2}`` at ``coverage=0.7`` returns ``[a]`` even though ``{a, c}`` fits with more members). Reworded to describe the actual algorithm and its non-largest-set trade-off. * :pr:`comment:3883483175` -- :func:`summarize_exclusion` skipped unknown ``excluded`` names in ``excluded_mass`` but counted them in ``num_excluded`` / ``num_remaining_quantized``, so ``scores={"a":5,"b":5}`` + ``excluded=["a","unknown"]`` yielded ``num_remaining_quantized == 0`` even though ``b`` remained quantized. Duplicate excluded names skewed the counts too. Filter ``excluded`` to unique names in ``scores`` before counting so all four fields stay consistent. Updated ``test_missing_node_names_default_zero`` (previously pinned the buggy behaviour) and added ``test_duplicate_excluded_names_counted_once``. * :pr:`comment:3883483188` -- ``test_failed_probe_is_recorded`` ``monkeypatch.setattr(score_mod, "quantize", ...)`` targeted the package-level re-exported function object (which ``score_mod`` was bound to via ``from modelopt.onnx.quantization.sensitivity import score``), not the ``modelopt.onnx.quantization.sensitivity.score`` module namespace that :func:`score` actually resolves ``quantize`` from. The fake was never called. Fixed by importing the implementation submodule directly and patching that module's ``quantize`` binding. Also added ``test_failed_probe_records_exceptions`` covering the sibling code path where ``quantize()`` raises rather than silently no-ops. Package structure ================= * :pr:`comment:3883483140` -- ``sensitivity/__init__.py`` converted to the ``from .module import *`` re-export pattern used by ``modelopt/torch/quantization/qtensor/`` and other subpackages, keeping the file-scope ``# ruff: noqa: F405`` and the explicit ``__all__`` public-contract list (same shape as ``modelopt/torch/quantization/utils/__init__.py``). ``picker.py`` gained an ``__all__`` list so its wildcard re-export ships exactly ``suggest_exclusion`` + ``summarize_exclusion``. CLI hardening ============= * :pr:`comment:3883483152` -- directory-mode ``--calibration_data_path`` bypassed size validation entirely, and ``_load_calibration_from_path`` then loaded every ``.npz`` shard under it and concatenated the arrays without per-file or aggregate limits. Added ``_validate_calibration_dir()`` that enforces a per- file cap of ``_CALIB_MAX_SIZE_BYTES`` (4 GiB, same as the file-mode guard) and an aggregate cap of ``_CALIB_DIR_MAX_TOTAL_BYTES`` (16 GiB). Empty directories now raise ``FileNotFoundError``. Logging (partial) ================= * :pr:`comment:3883483182` -- scrubbed the raw ``onnx_path`` from the scan-start log line and moved raw exception text to a ``logger.debug(..., exc_info=True)`` on the ``quantize()`` failure path. The per-probe warning still surfaces the exception **class** and the target name (target names are load-bearing for debugging; callers who want them redacted can filter at the log-handler layer). pyproject.toml ============== Registered the ``slow`` and ``slow_gpu`` markers in ``[tool.pytest.ini_options] markers = [...]`` so ``--strict-markers`` (project default via ``addopts``) doesn't fail collection on ``@pytest.mark.slow`` / ``@pytest.mark.slow_gpu`` decorators. These were already used on the CoAtNet integration tests but the marker declarations had never been included in a shipping commit. Deferred (author call, documented in the review threads) ======================================================== * :pr:`comment:3883483180` -- public-API artifact size guards on :func:`score`. Requires expanding the API contract (raise on legitimately large ONNX inputs OR plumb a trust knob through). Not in scope for this PR. * Target-name scrubbing from log messages -- keeping them is a deliberate debuggability choice. Signed-off-by: gcunhase <4861122+gcunhase@users.noreply.github.com> Co-Authored-By: modelopt-fix-agent-bot (Opus 4.7) <noreply@anthropic.com>