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

Add stubs for pycocotools #9086

Merged
merged 35 commits into from
Nov 28, 2022
Merged

Add stubs for pycocotools #9086

merged 35 commits into from
Nov 28, 2022

Conversation

hoel-bagard
Copy link
Contributor

This PR adds stubs for pycocotools (see #9061).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

Unfortunately you won't be able to use numpy as a dependency just yet. This should hopefully be changing soon; see #5768.

For now, as a workaround, I recommend you do something similar to what we do here:

@github-actions

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

@AlexWaygood Thank you for the workaround. I've removed the use of numpy, but if possible I'd like to leave the numpy typing information in case it can be used in the future. I'm not sure if it's better leave all the original lines as comments like here or to create aliases for every type I'm using like here.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 4, 2022

You can probably actually just replace the import line with a TypeAlias to Incomplete:

- import numpy as np
- import numpy.typing as not
+ # TODO when #5768 is resolved
+ # import numpy as np
+ # import numpy.typing as npt
+ np: TypeAlias = Incomplete
+ npt: TypeAlias = Incomplete

And then have the rest of the stub as it would be if you were actually importing numpy as np. I'm not 100% sure if that'll work, but might be worth a shot?

(TypeAlias needs to be imported from typing_extensions, BTW. And you'll need to use _typeshed.Self instead of typing(_extensions).Self for now, since mypy doesn't support typing(_extensions).Self yet. Note that _typeshed.Self is just a normal TypeVar, so it works slightly differently to typing(_extensions).Self.)

@github-actions

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

I've tried replacing the import line with a TypeAlias to Incomplete, but it doesn't work. I get errors like these:

stubs/pycocotools/pycocotools/coco.pyi:41: error: Name "npt.NDArray" is not defined
stubs/pycocotools/pycocotools/coco.pyi:41: error: Name "np.float64" is not defined

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 4, 2022

I've tried replacing the import line with a TypeAlias to Incomplete, but it doesn't work. I get errors like these:

stubs/pycocotools/pycocotools/coco.pyi:41: error: Name "npt.NDArray" is not defined
stubs/pycocotools/pycocotools/coco.pyi:41: error: Name "np.float64" is not defined

Okay, what you've done looks fine, in that case. It would be good to include a reference to #5768 in a comment(s) somewhere, though — that way we'll find it when we're grepping through the codebase after #5768 is solved

@github-actions

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

@AlexWaygood Two checks are still failing, but I don't understand why or how to fix it. Would you have time to look at it ?

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 5, 2022

Sure, I'll take a look soon!

@AlexWaygood AlexWaygood self-requested a review November 5, 2022 12:00
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Stubtest is a tool that compares the stub to the runtime, and verifies that the two are consistent with each other. Currently stubtest is unhappy about a few imports that don't look like they exist at runtime, and a file that doesn't exist at runtime :)

stubs/pycocotools/pycocotools/__init__.pyi Outdated Show resolved Hide resolved
stubs/pycocotools/pycocotools/coco_types.pyi Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

Thank you!
I've done as you instructed and now all the checks are passing. Is there anything else that needs to be modified ?

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 7, 2022

Thanks! I still need to do a manual review, but it looks like we're nearly there :)

@AlexWaygood AlexWaygood self-requested a review November 7, 2022 07:36
@hoel-bagard
Copy link
Contributor Author

Hi, do you have an idea of when you will be able to do the review ? (anytime is fine, I'm just wondering what I should expect)

In the stub I made, the part I'm the most unsure about this the use of _AnnotationG in stubs/pycocotools/pycocotools/coco.pyi. One of the methods of the package (annToRLE) returns a different type depending on the type of one of the element from the input dictionary, and I'm not sure how that should be typed.

@github-actions

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

@AlexWaygood

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, @hoel-bagard, and I'm sorry I've taken so long in getting to this. I've been busier than I expected the last few weeks :(

This mostly looks good, but I've left a few review comments. In general, we need to be quite cautious about using concrete types for parameter annotations. Abstract types are usually better -- they mean we don't run into annoying issues around invariance. There's some documentation on this here:

stubs/pycocotools/pycocotools/coco.pyi Outdated Show resolved Hide resolved
stubs/pycocotools/pycocotools/coco.pyi Outdated Show resolved Hide resolved
stubs/pycocotools/pycocotools/coco.pyi Outdated Show resolved Hide resolved
stubs/pycocotools/pycocotools/coco.pyi Outdated Show resolved Hide resolved
stubs/pycocotools/pycocotools/coco.pyi Outdated Show resolved Hide resolved
stubs/pycocotools/pycocotools/coco.pyi Outdated Show resolved Hide resolved
Comment on lines +16 to +20
def iou(
dt: _NDArrayUInt32 | list[float] | list[_EncodedRLE],
gt: _NDArrayUInt32 | list[float] | list[_EncodedRLE],
pyiscrowd: list[int] | _NDArrayUInt8,
) -> list[Any] | _NDArrayFloat64: ...
Copy link
Member

Choose a reason for hiding this comment

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

(No change requested, just pointing out to clarify:) This function should have the parameters annotated as list, since the code explicitly forbids other kinds of sequences: https://github.com/ppwwyyxx/cocoapi/blob/71e284ef862300e4319aacd523a64c7f24750178/PythonAPI/pycocotools/_mask.pyx#L182

hoel-bagard and others added 8 commits November 28, 2022 21:29
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood 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! Sorry again about the wait :)

@hoel-bagard
Copy link
Contributor Author

hoel-bagard commented Nov 28, 2022

Thank you for the great review process and all the advice !

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood merged commit a132ff2 into python:main Nov 28, 2022
hauntsaninja pushed a commit to python/mypy that referenced this pull request Dec 20, 2022
Removals from `stubinfo.py`:
- `atomicwrites` is archived and deprecated at runtime; stubs were
removed from typeshed in python/typeshed#8925
- `attrs` has had inline types for a very long time now
- `chardet` recently cut a release with inline types; typeshed's stubs
were marked obsolete in python/typeshed#9318
- `cryptography` has had inline types for a very long time now; the only
reason why it's still in typeshed is because other typeshed packages
need `types-cryptography` as a dependency, and our testing
infrastructure therefore can't currently cope with it being removed from
typeshed.
- `emoji` recently cut a release bundling stubs with the runtime
package; typeshed's stubs were marked obsolete in
python/typeshed#9051
- `termcolor` recently cut a release with inline types; typeshed's stubs
were marked obsolete in python/typeshed#8746
- `prettytable` recently cut a release with inline types; typeshed's
stubs were marked obsolete in
python/typeshed#9023

Additions:
- Stubs for `Xlib` were added in
python/typeshed#9279
- Stubs for `consolemenu` were added in
python/typeshed#8820
- Stubs for `dockerfile_parse` were added in
python/typeshed#9305
- Stubs for `flask_migrate` were added in
python/typeshed#8967
- Stubs for `paho.mqtt` were added in
python/typeshed#8853
- Stubs for `pycocotools` were added in
python/typeshed#9086
- Stubs for many `pywin32` modules were added in
python/typeshed#8825, and multiple follow-up PRs
- Stubs for `pyscreeze` were added in
python/typeshed#8823
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants