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

[Issue 7] Update import torchvision.models as models #26

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

gesuwen
Copy link
Contributor

@gesuwen gesuwen commented Feb 23, 2024

Create a new _UpdateTorchvisionModelsImports class to replace all "import torchvision.models as models" to "from torchvision import models"

@gesuwen gesuwen requested a review from kit1980 February 23, 2024 01:26
@gesuwen gesuwen self-assigned this Feb 23, 2024
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 23, 2024
@gesuwen
Copy link
Contributor Author

gesuwen commented Feb 23, 2024

@kit1980 I am aware that we could refactor the new class I added and _UpdateFunctorchImports into a generalized class for import replacement. Shall I do it here, or leave it for a future commit?

.gitignore Outdated Show resolved Hide resolved
@kit1980
Copy link
Contributor

kit1980 commented Feb 24, 2024

@gesuwen Thank you for the PR.

I've left some comments and I think in general this should be a separate rule with a separate code, like TOR201, to report the issue.
Just reporting the issue should be probably done first.
Changing the code can be done later, in a different PR even.

@gesuwen
Copy link
Contributor Author

gesuwen commented Mar 3, 2024

@gesuwen Thank you for the PR.

I've left some comments and I think in general this should be a separate rule with a separate code, like TOR201, to report the issue. Just reporting the issue should be probably done first. Changing the code can be done later, in a different PR even.

@kit1980 Thanks for your review! I just updated the PR, resolving all suggestions. Commit 2 move the new visitor to the vision dir and include giving LintViolation warning + replacement option. Could you please help review again and let me know if there is any addition suggestion?

@gesuwen gesuwen force-pushed the Add-a-rule-to-replace-torchvision.models branch 2 times, most recently from 610cba9 to b5fa0ea Compare March 4, 2024 22:40
@gesuwen gesuwen force-pushed the Add-a-rule-to-replace-torchvision.models branch from b5fa0ea to d117792 Compare March 4, 2024 22:41
@kit1980
Copy link
Contributor

kit1980 commented Mar 5, 2024

@gesuwen Looks good to me, thanks!

I'll do the codemod part later after factoring some things around import replacements.

@kit1980 kit1980 merged commit 8846f5c into main Mar 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants