Added pathlib support to datasets/utils.py #8200
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8200
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 09119e3 with merge base a00a72b ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ahmadsharif1
requested review from
pmeier and
NicolasHug
and removed request for
pmeier
torchvision/datasets/utils.py
Outdated
| from_path: Union[str, pathlib.Path], | ||
| to_path: Optional[Union[str, pathlib.Path]] = None, | ||
| remove_finished: bool = False, | ||
| ) -> pathlib.Path: |
There was a problem hiding this comment.
Unfortunately, forcing the return type to be a Path when it previously was a str is a BC-breaking change. It doesn't really matter for _decompress because it's private (leading underscore), but it's an issue here with extract_archive which is technically public.
(Side note: we didn't intend for any of the stuff in datasets/utils.py to ever be public, but that's another story).
We have 2 options here:
- always return a
str. This is sub-ideal but simpler. And since we never intended those utils to be public anyway, it's kinda OK - we should deprecate them and make them private eventually, but that's for another day. - always return the same type as
from_path. This is the best UX, and requires a bit more logic.
There was a problem hiding this comment.
Great point. I have done the second option (though technically speaking that also changes behavior from the public API).
There was a problem hiding this comment.
Thanks @ahmadsharif1 , one minor comment below, I'll let you decide before merging
Signed-off-by: Zoltán Böszörményi <zboszor@gmail.com> Co-authored-by: Nicolas Hug <nh.nicolas.hug@gmail.com>
Merging to sync the PR branch.
Reviewed By: vmoens Differential Revision: D55062771 fbshipit-source-id: a336ddeb8c1984fdb857f7b4583b0b464999a9dc
More work related to #8120 -- this time for all of datasets/utils.py.