Skip to content

fix(fx): drop the dynamic_range argument from the 8 remaining mark_as_int8_layer calls - #4651

Open
Anai-Guo wants to merge 2 commits into
pytorch:mainfrom
Anai-Guo:fix-fx-mark-as-int8-layer-arity
Open

fix(fx): drop the dynamic_range argument from the 8 remaining mark_as_int8_layer calls#4651
Anai-Guo wants to merge 2 commits into
pytorch:mainfrom
Anai-Guo:fix-fx-mark-as-int8-layer-arity

Conversation

@Anai-Guo

Copy link
Copy Markdown

Problem

13873bd "remove dynamic_range in fx" (#4221) changed

# py/torch_tensorrt/fx/converters/converter_utils.py:710
-def mark_as_int8_layer(layer, dynamic_range):
+def mark_as_int8_layer(layer):

and cleaned up adaptive_avgpool.py, impl/activation.py, maxpool.py,
quantization.py, transformation.py and one of the three sites in
linear.py. Eight call sites were missed and still pass two positional
arguments:

TypeError: mark_as_int8_layer() takes 1 positional argument but 2 were given

Replaying every mark_as_int8_layer(...) call site in fx/converters/ against
the signature parsed out of converter_utils.py on main:

main signature: (layer)

  TypeError add.py:47   (2 positional) -> mark_as_int8_layer() takes 1 positional argument but 2 were given
  TypeError add.py:64   (2 positional) -> ...
  TypeError add.py:70   (2 positional) -> ...
  TypeError batchnorm.py:21  (2 positional) -> ...
  OK        linear.py:27  (1 positional)
  TypeError linear.py:43  (2 positional) -> ...
  TypeError linear.py:51  (2 positional) -> ...
  TypeError mul.py:46   (2 positional) -> ...
  TypeError impl/convolution.py:130  (2 positional) -> ...

8 broken / 1 correct

Each one sits behind an is_quantized / scale is not None guard, so it is
reached by quantized add, add_relu, mul, BatchNorm, Linear and
Conv conversion through the FX path.

Fix

Same idiom #4221 used for linear.py:27: keep the call, drop the argument.
Where the argument was the only consumer of a get_dyn_range(...) result, the
computation and the now-unused import go too.

get_dyn_range itself is kept — quantization.py still uses it.

Note the two shapes #4221 dealt with: sites guarded by if input_val.dynamic_range:
were deleted whole (that attribute is gone in TRT 10), while sites guarded by
if is_quantized: kept the call. All eight sites here are the second kind, so
none of them are deleted.

Checks

black (26.3.1, the pinned pre-commit rev) reports all five files unchanged.
pyflakes is clean on the patched files — the one remaining note,
'typing.Any' imported but unused in impl/convolution.py, is pre-existing on
main and untouched here.

🤖 Generated with Claude Code

…_int8_layer calls

13873bd ("remove dynamic_range in fx", pytorch#4221) changed
mark_as_int8_layer(layer, dynamic_range) to mark_as_int8_layer(layer) and
cleaned up 6 files, but 8 call sites across 5 files still pass two positional
arguments and raise

    TypeError: mark_as_int8_layer() takes 1 positional argument but 2 were given

on every quantized conversion that reaches them: add.py:47/64/70,
batchnorm.py:21, linear.py:43/51, mul.py:46 and impl/convolution.py:130.
linear.py:27, the one site pytorch#4221 did update, is the intended shape.

Follows the same idiom as pytorch#4221: keep the call, drop the argument, and remove
the get_dyn_range() computations and imports that become unused. get_dyn_range
itself is kept -- quantization.py still uses it.

@micwill755 micwill755 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After this change, get_dyn_range has no remaining callers. #4221 already removed the last use in quantization.py and left only the import.

Could you also delete get_dyn_range from converter_utils.py and the unused import in quantization.py? It's leftover from the old ITensor.dynamic_range path which TRT 10 no longer exposes.

Per review: after dropping the extra `get_dyn_range(...)` argument from the
eight `mark_as_int8_layer` call sites, and after pytorch#4221 removed the last use in
quantization.py, `get_dyn_range` has no remaining callers. It is leftover from
the old `ITensor.dynamic_range` path that TRT 10 no longer exposes.

Delete the helper from converter_utils.py and drop the now-unused import in
quantization.py.
@Anai-Guo

Anai-Guo commented Sep 3, 2026

Copy link
Copy Markdown
Author

Thanks @micwill755 — done in 367eff1.

  • py/torch_tensorrt/fx/converters/converter_utils.py: removed get_dyn_range.
  • py/torch_tensorrt/fx/converters/quantization.py: dropped it from the from .converter_utils import ... line (the other name on that line, get_inputs_from_args_and_kwargs, is still used).

Confirmed there are no other references left in the repo — the only remaining hits for get_dyn_range before this commit were the eight mark_as_int8_layer call sites this PR already fixed, the definition itself, and that import.

py/torch_tensorrt/fx is excluded from both black (force-exclude) and isort (skip) in pyproject.toml, so the deletions are formatter-neutral.

@Anai-Guo

Anai-Guo commented Sep 4, 2026

Copy link
Copy Markdown
Author

Small note on the red checks here, since they are unrelated to this PR.

executorch-runtime-test / test / executorch-runtime-test--3.10-cu132 fails on a single assertion in tests/py/dynamo/executorch/test_api.py (1 failed, 220 passed):

FAILED executorch/test_api.py::test_runtime_wheel_pins_cuda_13_native_dependencies
  assert 'TENSORRT_DISTRIBUTION = "tensorrt-cu13"' in setup_source

That literal stopped existing on main in #4635 ("bring back cu126 support", 2026-09-02), which replaced the hard-coded constant with TENSORRT_DISTRIBUTION = tensorrt_distribution() in py/torch-tensorrt-executorch-runtime/setup.py. The test still greps for the old string, so it is red for every PR based on current main; #4659 already covers it.

This PR only touches py/torch_tensorrt/fx/converters/{converter_utils,quantization}.py and the fx tests, none of which the ExecuTorch runtime test imports. The gate failure is just the aggregate of that job.

Everything you asked for is in 367eff1; happy to rebase once #4659 lands if you would like a green run before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants