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

Re-enable or remove test_cpp_models.py? #3992

Closed
NicolasHug opened this issue Jun 7, 2021 · 3 comments
Closed

Re-enable or remove test_cpp_models.py? #3992

NicolasHug opened this issue Jun 7, 2021 · 3 comments

Comments

@NicolasHug
Copy link
Member

NicolasHug commented Jun 7, 2021

Currently, all the tests in test_cpp_models.py are unconditionally skipped:

@unittest.skipIf(
    sys.platform == "darwin" or True,
    "C++ models are broken on OS X at the moment, and there's a BC breakage on master; "
    "see https://github.com/pytorch/vision/issues/1191")

(note the or True).

I tried running them on my laptop and they all passed except for a few of the mnasnet tests that failed with RuntimeError: Given groups=32, expected weight to be divisible by 32 at dimension 0, but got weight of size [[40, 1, 3, 3]] instead , which is probably fairly easy to fix. BTW, I'm on OS X so the sys.platform == "darwin" can probably be removed too.

Should we fix and re-activate these tests? Or perhaps they are dedundant with the ones in test_models.py now? The tests ran in 2+ minutes for me, so reactivating them will have a significant impact on CI usage, which is not negligible.

CC @fmassa

cc @pmeier

@fmassa
Copy link
Member

fmassa commented Jun 9, 2021

Those tests test the C++ API of the models in torchvision. The current way we did to test them is not ideal, as we expose the C++ models to Python to compare the results of the baseline implementation in Python.

In principle we should enable the C++ tests back, but given that the C++ API of torchvision is in a state which it is not maintained (we don't enforce adding C++-equivalent models to the new Python models added), I'm tempted to not spend too much effort with it. If it's a quick fix, then great, but let's make sure we don't ship our torchvision binaries with the C++ models, and that it is only compiled in the unittests.

Additionally, it would be good to know who is the current maintainer of the C++ API of PyTorch

@NicolasHug
Copy link
Member Author

@fmassa , now that #4375 is merged and that the cpp API is deprecated, should we just remove the tests altogether? They're always skipped at the moment (and have been for a while)

@NicolasHug
Copy link
Member Author

I don't see this file anymore, we probably removed it

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

No branches or pull requests

2 participants