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

[NNCF][BC] Add test case with depthwise/transpose convolutions to template and backend specific tests #3004

Merged
merged 21 commits into from
Oct 16, 2024

Conversation

rk119
Copy link
Contributor

@rk119 rk119 commented Oct 8, 2024

Changes

  • Added simple Depthwise and Transpose Convolution models in tests/cross_fw/test_templates/helpers.py.

  • Updated map_references for Torch FX backend to assign the right reference node names for model classes Depthwise and Transpose Convolutions.

  • Added ONNXConvolutionTransposeMetatype into the list of OPERATIONS_WITH_BIAS for ONNX.

  • Added the missing target for Transpose Conv in transformations.py for Torch FX in the function _is_conv.

  • Added OVConvolutionBackpropDataMetatype into the list of OPERATIONS_WITH_BIAS for OpenVino backend

  • Replaced the unet graph in quantized reference graphs for FX backend.

Extra Changes

  • Update and finalize the right changes to make in tests/openvino/native/test_bias_correction.py for Transpose Convolution node name and accommodate for the changes in the name after each run.

Closes issue

#2916

@github-actions github-actions bot added NNCF PT Pull requests that updates NNCF PyTorch experimental NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF ONNX Pull requests that updates NNCF ONNX labels Oct 8, 2024
@rk119
Copy link
Contributor Author

rk119 commented Oct 10, 2024

Hi @daniil-lyakhov,

In the checks, the tests/torch/fx/test_models.py fails for the Unet model due to the change I made in transformations.py hence building a slightly different NNCF Graph. Prior to my change, the NNCF Graph with FX backend the param_constant nodes were before conv_transpose2d, after my update, it is after the conv_transpose2d hence causing a test failure.

image

The reference graph for Unet model has the old Nx Digraph which leads to a failure due to incompatible corresponding node names. The test compares the new nncf_graph structure now with the old reference nx digraph.

In my local repo, I have updated it to the new nx digraph and the test passes. Is this alright? Can I push this change to my PR?

@daniil-lyakhov
Copy link
Collaborator

daniil-lyakhov commented Oct 11, 2024

Hi @daniil-lyakhov,

In the checks, the tests/torch/fx/test_models.py fails for the Unet model due to the change I made in transformations.py hence building a slightly different NNCF Graph. Prior to my change, the NNCF Graph with FX backend the param_constant nodes were before conv_transpose2d, after my update, it is after the conv_transpose2d hence causing a test failure.

image

The reference graph for Unet model has the old Nx Digraph which leads to a failure due to incompatible corresponding node names. The test compares the new nncf_graph structure now with the old reference nx digraph.

In my local repo, I have updated it to the new nx digraph and the test passes. Is this alright? Can I push this change to my PR?

Hi @rk119,

please update the reference unet graph. Unet model has transpose convs, after your changes this transform convs and their biases are being separated. Test models fails were expected.

And please fix pre-commit tests, thanks!

@rk119
Copy link
Contributor Author

rk119 commented Oct 11, 2024

Hi @daniil-lyakhov,

I apologize if this seems redundant, but I would be grateful for your help with this issue I'm facing.

The following is the implementation of the Transpose Test model with just one convolution layer , as you are aware:

class TransposeConvTestModel(nn.Module):
    INPUT_SIZE = [1, 1, 3, 3]

    def __init__(self):
        super().__init__()
        with set_torch_seed():
            self.conv = create_transpose_conv(1, 2, 2, 1, 1, 2)
            self.conv.weight.data = torch.randn([1, 2, 2, 2])
            self.conv.bias.data = torch.randn([2])

    def forward(self, x):
        return self.conv(x)

In the Torch FX implementation of map_references, the parameters passed into this function are the reference biases and the PyTorch model. The Torch FX models and the NNCF graph consistently reproduce the same Transpose Convolution node name each time the test is run, as follows:

Torch FX model in test run 1:

image

Torch FX model in test run 2:

image

I have pointed out the node name we are using as the key to the dictionary, with corresponding bias values.

The map_references function retrieves the conv_transpose2d node for the Transpose test model. For the Multi Convolution model, the following Torch FX model is produced in each test run:

Test run 1:

image

Test run 2:

image

They are the same. The convolution node names after the first convolution have indexes after conv_2d_, starting from 1 and increasing incrementally. Hence, we obtain the index values using a simple algorithm.

However, in the OpenVINO backend, it is different. The first convolution node I am trying to access has a preceding index after the convolution name, but there is no discernible pattern and the indexes are inconsistent. I run the same test multiple times, and here's the model:

Test run 1:

image

Another test run:

image

The same Transpose Convolution model produces different node names in different runs of the same test file. I am still struggling to find a pattern to implement map_references for OpenVINO, where I can use an algorithm to obtain the index after the node name—unfortunately, I can't. However, I have two solutions to tackle this which works:

Solution 1:

I can keep the map_references implementation as it is

And access the exact node name by retrieving the first node with the ConvolutionBackpropData metatype from the NNCF graph in check_bias:

def check_bias(model: ov.Model, ref_biases: Dict) -> None:
        # this works
        nncf_graph = NNCFGraphFactory.create(model)
        for ref_name, ref_value in ref_biases.items():
            if ref_name == "/conv/ConvTranspose/WithoutBiases":
                node = nncf_graph.get_nodes_by_types(["ConvolutionBackpropData"])[0]
            else:
                node = nncf_graph.get_node_by_name(ref_name)
            ref_value = np.array(ref_value)
            curr_value = get_bias_value(node, nncf_graph, model)
            curr_value = curr_value.reshape(ref_value.shape)
            assert np.all(np.isclose(curr_value, ref_value, atol=0.0001)), f"{curr_value} != {ref_value}"
            

This works.

Solution 2:

Add another argument to map_references by passing the converted model to access the node name directly for the OpenVINO Transpose Conv model:

    @staticmethod
    def map_references(ref_biases: Dict, model_cls: Any, model: ov.Model) -> Dict[str, List]:
        mapping = {model.get_ops()[4].get_name() if model_cls is TransposeConvTestModel else f"{name}/WithoutBiases": val for name, val in ref_biases.items()}
        return mapping

These are the two solutions I can propose for now. I would be grateful if you could guide me on this. Thank you!

Another note:

This further extends to not being able to test Transpose Conv Test Model in test_verify_collected_stat_inputs_map since the node name in the converted ov_model as shown is different as well. When I include the Transpose Conv Test Model, the node name is different as seen below:

image

In conclusion, the checks fail for this test since the converted model has different indexes for the Convolution name for Transpose with no consistent pattern for me to recognize atleast.

@daniil-lyakhov
Copy link
Collaborator

daniil-lyakhov commented Oct 11, 2024

Hi @rk119,
Please use solution 1, just take the first node with transpose conv metatype in openvino tests.
Don't hesitate to just implement solutions and push them to the PR, we will check it during the code interview anyways

@rk119
Copy link
Contributor Author

rk119 commented Oct 11, 2024

Hi @rk119, Please use solution 1, just take the first node with transpose conv metatype in openvino tests.

Alright! Just did.

Don't hesitate to just implement solutions and push them to the PR, we will check it during the code interview anyways

Haha alright yes. :)

@rk119 rk119 marked this pull request as ready for review October 11, 2024 14:37
@rk119 rk119 requested a review from a team as a code owner October 11, 2024 14:37
@daniil-lyakhov daniil-lyakhov self-requested a review October 14, 2024 13:35
Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov left a comment

Choose a reason for hiding this comment

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

Great job, thank you!

@daniil-lyakhov
Copy link
Collaborator

@alexsu52, please merge

@alexsu52 alexsu52 requested a review from kshpv October 15, 2024 05:02
@alexsu52
Copy link
Contributor

@kshpv please, take a look at ONNX part.

Copy link
Collaborator

@kshpv kshpv left a comment

Choose a reason for hiding this comment

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

Hi @rk119,
Thank you for your contribution!
I noticed that you added FBC tests for Depthwise and Transpose cases. However, FBC does not apply corrections for these cases, so the proposed tests do not have any effect. I suggest removing these tests to avoid any confusion.

@rk119
Copy link
Contributor Author

rk119 commented Oct 15, 2024

Hi @rk119, Thank you for your contribution! I noticed that you added FBC tests for Depthwise and Transpose cases. However, FBC does not apply corrections for these cases, so the proposed tests do not have any effect. I suggest removing these tests to avoid any confusion.

Ohhh alright. I'll remove them. Thank you for clarifying :)

@rk119 rk119 changed the title [NNCF][FBC/BC] Add test case with depthwise/transpose convolutions to template tests [NNCF][BC] Add test case with depthwise/transpose convolutions to template tests Oct 15, 2024
@rk119 rk119 changed the title [NNCF][BC] Add test case with depthwise/transpose convolutions to template tests [NNCF][BC] Add test case with depthwise/transpose convolutions to template and backend specific tests Oct 15, 2024
@alexsu52 alexsu52 merged commit 17f799e into openvinotoolkit:develop Oct 16, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental NNCF ONNX Pull requests that updates NNCF ONNX NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PT Pull requests that updates NNCF PyTorch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants