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

Fix warnings in unit-tests tests #6159

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

puhuk
Copy link
Contributor

@puhuk puhuk commented Jun 13, 2022

Fixes #6155

Fix warnings in the tests
@datumbox datumbox requested a review from vfdev-5 June 13, 2022 15:21
@datumbox datumbox changed the title To resolve issue #6155 Fix warnings in unit-tests tests Jun 13, 2022
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jun 13, 2022

@puhuk ci failures are related.

I think a fix could be something like:

diff --git a/torchvision/models/quantization/googlenet.py b/torchvision/models/quantization/googlenet.py
index 644df8ae4..682ba1833 100644
--- a/torchvision/models/quantization/googlenet.py
+++ b/torchvision/models/quantization/googlenet.py
@@ -74,6 +74,8 @@ class QuantizableInceptionAux(InceptionAux):
 class QuantizableGoogLeNet(GoogLeNet):
     # TODO https://github.com/pytorch/vision/pull/4232#pullrequestreview-730461659
     def __init__(self, *args: Any, **kwargs: Any) -> None:
+        if "init_weights" not in kwargs:
+            kwargs["init_weights"] = True
         super().__init__(  # type: ignore[misc]
             blocks=[QuantizableBasicConv2d, QuantizableInception, QuantizableInceptionAux], *args, **kwargs
         )
diff --git a/torchvision/models/quantization/inception.py b/torchvision/models/quantization/inception.py
index ba4b21d41..647c00472 100644
--- a/torchvision/models/quantization/inception.py
+++ b/torchvision/models/quantization/inception.py
@@ -142,6 +142,7 @@ class QuantizableInception3(inception_module.Inception3):
                 QuantizableInceptionE,
                 QuantizableInceptionAux,
             ],
+            init_weights=True,
         )
         self.quant = torch.ao.quantization.QuantStub()
         self.dequant = torch.ao.quantization.DeQuantStub()

test/test_models.py Outdated Show resolved Hide resolved
Remove unrelevant init_weights from mobilenet2, shufflenet2
@puhuk
Copy link
Contributor Author

puhuk commented Jun 14, 2022

Hmm, why it raises typo error in test_models.py when I just revert to origin code

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jun 14, 2022

@puhuk
Copy link
Contributor Author

puhuk commented Jun 14, 2022

@vfdev-5 Thank you for the feedback!

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jun 14, 2022

@puhuk there are few similar warnings:

test/test_models.py::test_classification_model[cpu-googlenet]
  /root/project/torchvision/models/googlenet.py:47: FutureWarning: The default weight initialization of GoogleNet will be changed in future releases of torchvision. If you wish to keep the old behavior (which leads to long initialization times due to scipy/scipy#11299), please set init_weights=True.
    warnings.warn(

test/test_models.py::test_classification_model[cpu-inception_v3]
  /root/project/torchvision/models/inception.py:43: FutureWarning: The default weight initialization of inception_v3 will be changed in future releases of torchvision. If you wish to keep the old behavior (which leads to long initialization times due to scipy/scipy#11299), please set init_weights=True.
    warnings.warn(

Can you try to fix them as well ? Thanks

To resolve warnings in models/googlenet, models/inception
@@ -34,7 +34,7 @@ def __init__(
num_classes: int = 1000,
aux_logits: bool = True,
transform_input: bool = False,
init_weights: Optional[bool] = None,
init_weights: bool = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not good. Check #2170 why they switched from True to None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't have good idea. Is there better method than I did here.
https://github.com/pytorch/vision/pull/6159/commits/16ddd6c0f23fe56e8029f336c5e3f3c69e9c534f

To resolve warnings in models/googlenet, models/inception
@@ -74,6 +74,8 @@ def forward(self, x: Tensor) -> Tensor:
class QuantizableGoogLeNet(GoogLeNet):
# TODO https://github.com/pytorch/vision/pull/4232#pullrequestreview-730461659
def __init__(self, *args: Any, **kwargs: Any) -> None:
if "init_weights" not in kwargs:
kwargs["init_weights"] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why in order to fix the warning on unit-tests we modify the models themselves. My expectations without having all the details in my mind, is that the tests are the ones that need to be updated. What am I missing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

At first I was thinking that QuantizableGoogLeNet does not properly set up its base class GoogLeNet. Thinking more about that, I think you are right, we have to try again to fix that from tests side

Copy link
Collaborator

@vfdev-5 vfdev-5 Jun 15, 2022

Choose a reason for hiding this comment

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

@puhuk can you check if we could use _model_params when we set kwargs = {**defaults, **_model_params.get(model_name, {})}. Looks like _model_params can define specific values for special models:

vision/test/test_models.py

Lines 246 to 247 in 93b3e84

_model_params = {
"inception_v3": {"input_shape": (1, 3, 299, 299)},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me check and send PR again.

Update _model_params to prevent future warning
@datumbox
Copy link
Contributor

datumbox commented Jun 15, 2022

I think the approach taken now should work. Our CI is toasted so we won't be able to test this thoroughly, so I recommend waiting for tomorrow until the breakage is fixed.

Edit: The failing linter is related. You need to add a space at "googlenet":{

To correct format
@puhuk
Copy link
Contributor Author

puhuk commented Jun 15, 2022

@datumbox
Thanks for the review, let me wait till tomorrow :)
And I fix the error by formatting with black.

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.

Fix warnings in the tests
4 participants