If we not removed initialized input, should we keep initializer also?

[leshabirukov]

Me still haunted by #2211
We allow hanging inputs, because it is part of interface, but we are removing it's initializer if exist. May be we should keep it, because it is affects interface either, - now user can ignore it before DCE pass, but can not ignore after.

[justinchuby]

I am not following this. Could you share a concrete example?

[leshabirukov]

In the code below first run is successful, while second run gives

ValueError: Required inputs (['two']) are missing from input feed (['x']).

after remove_unused_nodes

import onnx
from onnxscript.optimizer import remove_unused_nodes
import onnxruntime as ort
import numpy as np


def util_random( shape ):
    return np.round(np.random.random( shape )*10).astype(np.float32)

def test_unused_initialized_inputs_are_kept_by_default():
    model = onnx.parser.parse_model(
        """
        <ir_version: 10, opset_import: [ "" : 17]>
        agraph (float[2] x, float[2] two) => (float[2] z)
        <float[2] two = {2.0,2.0}> {
            four = Add(two, two)
            z = Mul(x, x)
        }
    """
    )
    inputTensor = util_random([2], )
    print(inputTensor)
    session_1 = ort.InferenceSession(model.SerializeToString())
    session_1.run(None, {"x": inputTensor})

    remove_unused_nodes(model)

    session_2 = ort.InferenceSession(model.SerializeToString())
    session_2.run(None, {"x": inputTensor})


test_unused_initialized_inputs_are_kept_by_default()

[justinchuby]

So I guess we need to keep the initializer values as well, right?

[leshabirukov]

Notes:

• Doing DCE It would be good to warn user, he have hanging initialized outputs, proposing to turn remove_initialized_inputs on
• Initializers consume space, may be some workaround to make it smaller (optionally)? Relax (erase) input shape with replacing initializer with tiny one, (not sure it will work this way).

[justinchuby]

1. A warning sounds good!
2. When a user want to keep the inputs they probably want to maintain the input shape / signature as well so maybe we will have to retain the initializers as is

[leshabirukov]

Should it be warnings.warn or logger.warning?

[justinchuby]

logger.warning is preferable

[leshabirukov]

Done.
Now I'm annoyed by this marginal functionality all over RemoveUnusedNodesPass::call. Move it to a separate function?

[justinchuby]

Sounds reasonable. Maybe create a pass that is "remove unused inputs"?

[leshabirukov]

Err, I think rather about just intrnal function in RemoveUnusedNodesPass, anyway, warning belongs there.

[leshabirukov]

count += 1
Should we count initialized input as 1 or 2?

[justinchuby]

count += 1 Should we count initialized input as 1 or 2?

Whichever is the most simple.

[leshabirukov]

My apologies, can't run linters here

[gramalingam]

Sorry about the delayed response ... didn't see this thread when looking at the PR.

I agree with the initial conclusion (by both of you): since this is part of the contract with the external world (callers), it seems best to keep the same set of inputs ... and not to remove the initializer when it is also an input.

Removing the initializer/input can be a separate (semantics-altering) transformation, not part of DCE which is supposed to preserve behavior