Skip to content

Conversation

ngoldbaum
Copy link
Contributor

Back in April, @malmaud added type annotations for dataloader.py. However, at about the same time, @ssnl in #19228 replaced _DataLoaderIter with _BaseDataLoaderIter and two subclasses, _SingleProcessDataLoaderIter, and _MultiProcessingDataLoaderIter. However - probably because these changes happened in parallel at roughly the same time, the type stubs and several other references in the codebase were never updated to match this refactoring.

I've gone ahead and done the updates to reflect the refactoring in #19228, which fixes the specific type stub/impelementation mismatch pointed out in #26673, although not the broader problem that pytorch doesn't have a test to make sure that the .pyi type stub files match the real API defined in .py files.

@ngoldbaum ngoldbaum requested a review from ezyang September 30, 2019 22:12
@ngoldbaum ngoldbaum requested a review from apaszke as a code owner September 30, 2019 22:12
@pytorchbot pytorchbot added module: dataloader Related to torch.utils.data.DataLoader and Sampler module: internals Related to internal abstractions in c10 and ATen module: typing Related to mypy type annotations labels Sep 30, 2019
Copy link
Collaborator

@ssnl ssnl left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Has the problem mentioned in previous comment been solved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so? I wasn’t able to reproduce the error in the comments with the latest version of mypy as well as a version from April, I was running:

mypy torch/utils/data/dataloader.py

and didn’t see any errors from mypy. That invocation may not have been sufficient to trigger the error @malmaud saw when they originally wrote this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point relatively recently, Python/mypy added the ability to handle unquoted forward references, but there are definitely versions which cannot. I think it's safer to keep the quotes unless I was actually in error before and this isn't a forward reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ngoldbaum mind updating this? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I also expanded the comment to reflect the current situation in mypy so future readers are hopefully a little less confused.

sublcasses `_SingleProcessDataLoaderIter` and
`_MultiprocessingDataLoaderIter`. However, the type stubs were not
updated at the same time. This renames the stub for `_DataLoaderIter`
with the new name for the base class.
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

waiting on ci

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in f522bde.

thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
…ch#27105)

Summary:
Back in April, malmaud added type annotations for `dataloader.py`. However, at about the same time, SsnL in pytorch#19228 replaced `_DataLoaderIter` with `_BaseDataLoaderIter` and two subclasses, `_SingleProcessDataLoaderIter`, and `_MultiProcessingDataLoaderIter`. However - probably because these changes happened in parallel at roughly the same time, the type stubs and several other references in the codebase were never updated to match this refactoring.

I've gone ahead and done the updates to reflect the refactoring in pytorch#19228, which fixes the specific type stub/impelementation mismatch pointed out in pytorch#26673, although not the broader problem that pytorch doesn't have a test to make sure that the `.pyi` type stub files match the real API defined in `.py` files.
Pull Request resolved: pytorch#27105

Differential Revision: D17813641

Pulled By: ezyang

fbshipit-source-id: ed7ac025c8d6ad3f298dd073347ec83bb4b6600c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: dataloader Related to torch.utils.data.DataLoader and Sampler module: internals Related to internal abstractions in c10 and ATen module: typing Related to mypy type annotations open source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants