-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Document that datasets support pathlib.Path #8321
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8321
Note: Links to docs will display an error until the docs builds have been completed. ❌ 16 New Failures, 21 Unrelated FailuresAs of commit fdf85eb with merge base 0325175 (): NEW FAILURES - The following jobs have failed:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Can you post here some of the mypy errors you see with imagenet and updated typing hints? |
Sorry, mypy takes 3+ minutes to run on my machine and honestly I just don't want to go back to it knowing that I won't be addressing type annotations for all those 40+ files anyway. |
de2e085
to
01977a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the type annotations. Regarding testing, #8320 is what I would have done as well. If you want the extra security, we could potentially implement a smoke test for that, but not sure if its worth it.
BC linter fails with these warnings:
Yes, annotations have changed, but they were expanded. Passing |
Here is the green |
Thanks @pmeier |
Hey @NicolasHug! You merged this PR, but no labels were added. |
Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de> Reviewed By: vmoens Differential Revision: D55062786 fbshipit-source-id: c2b92f640113ef5a4cef4a91187da720f5945baa
All datasets'
root
param already supportspathlib.Path
because they just callos.expanduser()
under the hood, which will nicely handlePath
. So this PR just adds that to the docstrings.This isn't explicitly tested, but #8320 should prove that
Path
works fine (@pmeier if there's a better way to test that consistently, LMK).I'm not updating the type annotations because in all honestly I really, REALLY don't want to spend my time doing that. I gave it a go adding the annotations mentioned in #8120 (comment) to just the imagenet file, and after running for 3+ minutes mypy started to give me hell. So I'm just not doing it. If someone wants to spend the time and add the annotations, I'll be happy to stamp the PR.
With that and #8314, #8120 could be closed.