Skip to content

[Nova] More comprehensive Smoke Tests for Torchvision #6803

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

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

osalpekar
Copy link
Member

@osalpekar osalpekar commented Oct 20, 2022

Adding more detailed smoke tests to verify nightly and release binaries. Currently, only import torchvision is run, and that too is disabled in some builds in CircleCI. We add this new script that imports publicly exposed modules in the torchvision package. Eventually we will run this script in the GHA workflow that builds the binaries to ensure they are healthy. This list was derived from running sys.modules.keys() on the pip-installed package and filtering out all non-vision modules.

Would love general feedback, as well as whether we want the private modules (prefixed with _) to be included.

cc @seemethere

@pmeier
Copy link
Collaborator

pmeier commented Oct 20, 2022

Not sure what the point is here. import torchvision imports our namespaces automatically

from torchvision import datasets, io, models, ops, transforms, utils

There is no need to import everything manually.

@osalpekar
Copy link
Member Author

osalpekar commented Oct 21, 2022

Not sure what the point is here. import torchvision imports our namespaces automatically

from torchvision import datasets, io, models, ops, transforms, utils

There is no need to import everything manually.

@pmeier Ah thanks for the catch! I can remove all of these imports and just keep the line that prints the version. I did walk through the __init__.py files in each of the modules, but did want to double-check one thing: are any modules/methods lazily loaded? For example, things like StreamReader within torchaudio.io are lazily loaded (https://github.com/pytorch/audio/blob/main/torchaudio/io/__init__.py), so we must do an explicit check to ensure that import works. Is there any such import in torchvision?

@datumbox
Copy link
Contributor

@YosuaMichael Are there any smoke tests that we can run here? I know you worked a bit on the revamping of the tests but not sure if you got to the point to split a good set of smoke tests.

@pmeier
Copy link
Collaborator

pmeier commented Oct 21, 2022

are any modules/methods lazily loaded?

I'm not aware of any, but let's ask @NicolasHug @YosuaMichael @jdsgomes if something hides in our image and video I/O namespaces.

@YosuaMichael
Copy link
Contributor

@pmeier AFAIK, we dont do lazy loading on IO spaces, so I agree that import torchvision should be enough.

@datumbox as of now I haven't got test for the smoke test, I plan to focus more on CI after the release (so this should not block this PR).

@osalpekar
Copy link
Member Author

Ok, thanks for the insight all! Given there are no lazy uploads, I'll make the smoke tests simply import torchvision and print the package version.

@pmeier
Copy link
Collaborator

pmeier commented Oct 26, 2022

@osalpekar I'm not sure I understand what this file is trying to achieve, or rather why we need a file for this. Could you post where it is used or where you plan to use it? Otherwise, we can probably remove it and replace it with

python -c "import torchvision; print(torchvision.__version__)"

which does the same.

@osalpekar
Copy link
Member Author

@osalpekar I'm not sure I understand what this file is trying to achieve, or rather why we need a file for this. Could you post where it is used or where you plan to use it? Otherwise, we can probably remove it and replace it with

python -c "import torchvision; print(torchvision.__version__)"

which does the same.

This will be used as we move binary build/release workflows from CircleCI to GitHub Actions. The base GitHub Actions are implemented in the pytorch/test-infra repo. Each domain (vision, text, etc.) will simply call into these base workflows to build and upload wheels/conda binaries across the whole build matrix (these caller workflows have not been added for vision yet, but will be soon). We have a step that runs smoke tests on the built binaries prior to uploading them to conda/pip, where each domain supplies its own smoke-test script. This file will function as that smoke test script. Having this as a standalone script will follow the conventions from the other domains and keep the base GHA simpler and more readable.

@pmeier
Copy link
Collaborator

pmeier commented Oct 27, 2022

Makes sense to have a file then. Still, I don' quite understand why we need to print the version. This call could fail in case torchvision.__version__ is not correctly set up. Given that this works, what is the benefit of the printing? For manual verification? Should we rather compare it to the known version?

these caller workflows have not been added for vision yet, but will be soon

Do we need this PR before the workflows will be enabled? Otherwise, could you link them before we merge for review?

@osalpekar
Copy link
Member Author

Linked the caller workflows: #6855 (Line Wheels) and #6856 (Linux Conda). The version print is a minor sanity check (and breadcrumb for debugging that our release engineers find useful when cutting the domain lib releases).

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks @osalpekar!

@osalpekar osalpekar merged commit c32bec5 into pytorch:main Oct 28, 2022
@github-actions
Copy link

Hey @osalpekar!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request Oct 31, 2022
Summary:
* [Nova] More comprehensive Smoke Tests for Torchvision

* No need to import everything since nothing is lazily uploaded

Reviewed By: datumbox

Differential Revision: D40851026

fbshipit-source-id: 165ee117a8d5aa33c3af746289a3f0755753e7d9
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.

5 participants