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

fix mypy on VisionDataset #8134

Merged
merged 1 commit into from
Nov 23, 2023
Merged

fix mypy on VisionDataset #8134

merged 1 commit into from
Nov 23, 2023

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Nov 23, 2023

Supersedes #8128. Error caused by #8124.

Copy link

pytorch-bot bot commented Nov 23, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8134

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (3 Unrelated Failures)

As of commit 9ae3188 with merge base 83090c2 (image):

BROKEN TRUNK - The following jobs 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.

@@ -31,7 +31,7 @@ def __init__(
target_transform: Optional[Callable] = None,
random_offset: int = 0,
) -> None:
super().__init__(None, transform=transform, target_transform=target_transform) # type: ignore[arg-type]
super().__init__(transform=transform, target_transform=target_transform)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Driveby, since we no longer need to pass None after #8124.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks Philip. One Q but LGTM anyway

@@ -29,7 +29,7 @@ class VisionDataset(data.Dataset):

def __init__(
self,
root: Optional[str] = None,
root: str = None, # type: ignore[assignment]
Copy link
Member

Choose a reason for hiding this comment

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

can we still keep the Optional[str] type which is technically correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can't. If we use Optional[str] as annotation, mypy correctly infers that for the self.root attribute. And with that we are back at the issues we are currently seeing, i.e. all child datasets that perform anything with self.root would now need to check that self.root is in fact a str and not None.

@pmeier
Copy link
Collaborator Author

pmeier commented Nov 23, 2023

@pmeier pmeier merged commit 3eb4333 into pytorch:main Nov 23, 2023
61 of 64 checks passed
@pmeier pmeier deleted the fix-mypy branch November 23, 2023 20:31
facebook-github-bot pushed a commit that referenced this pull request Jan 15, 2024
Reviewed By: vmoens

Differential Revision: D52539016

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

3 participants