Skip to content

Add type hint stub #780

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

Closed
wants to merge 1 commit into from
Closed

Add type hint stub #780

wants to merge 1 commit into from

Conversation

ejguan
Copy link
Contributor

@ejguan ejguan commented Sep 19, 2022

Fixes #779

Changes

  • Add py.typed file stub
  • Include the file into package data

@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 Sep 19, 2022
@ejguan ejguan marked this pull request as ready for review September 19, 2022 14:34
@facebook-github-bot
Copy link
Contributor

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pmeier
Copy link
Contributor

pmeier commented Sep 20, 2022

This breaks torchvision pretty hard: https://app.circleci.com/pipelines/github/pytorch/vision/20354/workflows/861e7e2c-4e0b-4594-9d81-33f0b2d27f43/jobs/1655543

Found 299 errors in 26 files (checked 219 source files)

I haven't looked in detail, but some of the errors don't seem to be a usage error. I'll keep you posted.

@pmeier
Copy link
Contributor

pmeier commented Sep 20, 2022

An example where I don't think we are responsible

from typing import BinaryIO, Tuple
import pathlib

from torchdata.datapipes.iter import FileLister, FileOpener, IterDataPipe, IterableWrapper


def load(path: pathlib.Path) -> IterDataPipe[Tuple[str, BinaryIO]]:
    if path.is_dir():
        dp = FileLister(str(path), recursive=True)
    else:
        dp = IterableWrapper([str(path)])
    return FileOpener(dp, mode="rb")

Running mypy on this yields

main.py:11: error: Incompatible types in assignment (expression has type "IterableWrapperIterDataPipe",
variable has type "FileListerIterDataPipe")  [assignment]
            dp = IterableWrapper([str(path)])
                 ^
main.py:12: error: Incompatible return value type (got "FileOpenerIterDataPipe", expected
"IterDataPipe[Tuple[str, BinaryIO]]")  [return-value]

The actual function is a little more complicated and we can't use FileOpener as return type, whereas IterDataPipe[Tuple[str, BinaryIO]] is true for all cases.

Could you advise how you want us to annotate? Can we maybe revert this PR and resolve all issues that this brings to torchvision before we merge a second time?

@ejguan
Copy link
Contributor Author

ejguan commented Sep 20, 2022

@pmeier
At list for the first Error, the proper typing should be:

def load(path: pathlib.Path) -> IterDataPipe[Tuple[str, BinaryIO]]:
    if path.is_dir():
        dp: IterDataPipe = FileLister(str(path), recursive=True)
    else:
        dp = IterableWrapper([str(path)])
    return FileOpener(dp, mode="rb")

However, even with the proper typing shown above, the Error is changed to Incompatible types in assignment (expression has type "FileListerIterDataPipe", variable has type "IterDataPipe[Any]"). And, it doesn't explain what causes the second Error.
So, I spent a few hours figuring out what is the root cause of the mypy Error. In the generated datapipe.pyi file, a new IterDataPipe class is defined, which overrides the original IterDataPipe from the inheritance graph for all other DataPipe.

All Errors are eliminated when I remove new IterDataPipe definition from datapipe.pyi and import IterDataPipe directly from torch.utils.data.datapipe. And, the reason we define new IterDataPipe in pyi file is attaching all functional APIs to it. We need to do it in a different way by keeping the original IterDataPipe and extending the class with all functional APIs.

cc: @NivekT for python interface file

For this PR, I will revert it because our typing system needs to be fixed generally.

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by b87e016.

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. Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include py.typed marker file for mypy type hint compatibility
5 participants