[L] Change in code/re2[main]: add type stubs and mypy.ini
Daniel McClanahan (Gerrit)
Daniel McClanahan added 1 comment![]( "Open in Gerrit")
This change doesn't add anything to the bazel build yet, but I was thinking that getting type stubs to work at all would establish a nice ground truth before getting them to work in bazel.
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Wankadia (Gerrit)
Paul Wankadia added 13 comments![]( "Open in Gerrit")
This change doesn't add anything to the bazel build yet, but I was thinking that getting type stubs to work at all would establish a nice ground truth before getting them to work in bazel.
Quoting https://github.com/google/re2/issues/496#issuecomment-2166522270:
If the Bazel community ever decides to make Python type checkers Just Work™, we can revisit this then. In the meantime, I have no desire to (co-)author and subsequently maintain whatever Starlark would be needed, so please don't spend any (more!) time worrying about it. ;)
Thank you so much for doing this! I have left a first round of comments. I haven't actually looked at the `.pyi` files yet.
Please revert `python.yml` and, instead, replace the TODO in `ci-bazel.yml` with something like the following:
```
- name: Test stubs
run: |
DIR=$(mktemp -d)
cp bazel-bin/python/re2_test.runfiles/_main/python/* "${DIR}"
cp python/*.pyi python/mypy.ini "${DIR}"
cd "${DIR}"
mypy .
shell: bash
```
`python.yml` is a release workflow, not a CI workflow. Moreover, testing the wheels is important for catching any platform-specific breakage before publishing to PyPI, but the stubs aren't going to break like that. (Well, I certainly hope they won't.)
warn_return_any = True
warn_unused_configs = TrueCan we just set `strict` (or a similar knob) to enable everything and then, if there are problems, squelch them selectively and leave comments to explain what still needs to be fixed?
disallow_untyped_defs = TrueI would prefer not to do this; see the comment in `re2_test.py`.
ignore_missing_imports = True`ci-bazel.yml` now installs `absl-py` as well as `mypy`, so this should hopefully not be needed.
Do we need to add type annotations throughout this file in order to get mypy to do anything useful for us? There seems to be a lot of noise (almost entirely around the `test_*()` methods) for what feels like very little value.
assert match is not NonePlease use `self.assertIsNotNone(…)` here and below.
with open('re2.py', 'r') as file:
re2_contents = relativize_native_import(file.read())
with open('re2/__init__.py', 'x') as file:
file.write(re2_contents)```suggestion
with open('re2.py', 'r') as file:
contents = relativize_native_import(file.read())
with open('re2/__init__.py', 'x') as file:
file.write(contents)
```
with open('re2.pyi', 'r') as file:
re2i_contents = relativize_native_import(file.read())
with open('re2/__init__.pyi', 'w') as file:
file.write(re2i_contents)```suggestion
with open('re2.pyi', 'r') as file:
contents = relativize_native_import(file.read())
with open('re2/__init__.pyi', 'x') as file:
file.write(contents)
```
shutil.copyfile('_re2.pyi', 're2/_re2.pyi')```suggestion
shutil.copyfile('_re2.pyi', 're2/_re2.pyi')
with open('re2/py.typed', 'x') as file:
pass
```Related details
- Daniel McClanahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 6 comments![]( "Open in Gerrit")
Please revert `python.yml` and, instead, replace the TODO in `ci-bazel.yml` with something like the following:
```
- name: Test stubs
run: |
DIR=$(mktemp -d)
cp bazel-bin/python/re2_test.runfiles/_main/python/* "${DIR}"
cp python/*.pyi python/mypy.ini "${DIR}"
cd "${DIR}"
mypy .
shell: bash
````python.yml` is a release workflow, not a CI workflow. Moreover, testing the wheels is important for catching any platform-specific breakage before publishing to PyPI, but the stubs aren't going to break like that. (Well, I certainly hope they won't.)
So the reason I wanted to test stubs like that (in the separate dir, along with `re2_test.py`, with the wheel) was to ensure that the specific little magic we've added in `setup.py` (renaming the `.pyi` files and regex replacing to `from . import _re2`) would correctly "import" with mypy when users consume the output wheel.
I totally agree that there's no platform-specific testing to perform here, but in case we decide e.g. to (heaven forfend) add further `.pyi` files in the future (which e.g. may also need renaming and regex replacing imports), I *would* like to have a single test (not platform-specific!) which imports the wheel, and then runs mypy.
What you've written out here is a correct invocation of mypy that corresponds to the way I've been testing locally, but I *also* want to make sure our types work in the mypy context when imported from the wheel.
This is very long-winded (sorry!), but what I'll do is add this "Test stubs" phase you've helpfully written for me, and then I'll try to add a phase that executes mypy when importing types from the wheel (in the CI and not the release workflow).
warn_return_any = True
warn_unused_configs = TrueCan we just set `strict` (or a similar knob) to enable everything and then, if there are problems, squelch them selectively and leave comments to explain what still needs to be fixed?
Working on this! I do not believe there is a `strict` option and instead we need to turn on a million flags \^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^ https://mypy.readthedocs.io/en/stable/config_file.html I will research further!
exclude = (?x)(
setup\.py
)Not needed?
This is definitely necessary when running `mypy` locally. I also have an additional exclusion for that case.
ignore_missing_imports = True`ci-bazel.yml` now installs `absl-py` as well as `mypy`, so this should hopefully not be needed.
This *was* necessary when I ran `mypy` locally before, but appears to no longer be necessary! Fixed!
from __future__ import annotationsNot needed?
This little trinket is necessary to get some behavior we probably want (see https://mypy.readthedocs.io/en/stable/runtime_troubles.html and https://peps.python.org/pep-0563/#enabling-the-future-behavior-in-python-3-7), for pythons under 3.12. However, it seems to only be necessary for `.py` files, so I will remove it from the `.pyi`!
assert match is not NonePlease use `self.assertIsNotNone(…)` here and below.
You're going to hate this, but I'm pretty confident this is literally necessary for mypy, which is able to recognize `x is not None`, but is not able to infer that `self.assertIsNotNone(x)` means `x` is not `None`. With `self.assertIsNotNone(match)`, we get:
```
re2_test.py:172: error: Item "None" of "Match[str] | None" has no attribute "span" [union-attr]
re2_test.py:172: error: Item "None" of "Match[bytes] | None" has no attribute "span" [union-attr]
re2_test.py:172: error: Item "None" of "Match[str] | None" has no attribute "re" [union-attr]
re2_test.py:172: error: Item "None" of "Match[bytes] | None" has no attribute "re" [union-attr]
```
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 3 comments![]( "Open in Gerrit")
Daniel McClanahanNot needed?
This is definitely necessary when running `mypy` locally. I also have an additional exclusion for that case.
Done
Daniel McClanahan`ci-bazel.yml` now installs `absl-py` as well as `mypy`, so this should hopefully not be needed.
This *was* necessary when I ran `mypy` locally before, but appears to no longer be necessary! Fixed!
Done
Daniel McClanahanNot needed?
This little trinket is necessary to get some behavior we probably want (see https://mypy.readthedocs.io/en/stable/runtime_troubles.html and https://peps.python.org/pep-0563/#enabling-the-future-behavior-in-python-3-7), for pythons under 3.12. However, it seems to only be necessary for `.py` files, so I will remove it from the `.pyi`!
Done
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 1 comment![]( "Open in Gerrit")
Do we need to add type annotations throughout this file in order to get mypy to do anything useful for us? There seems to be a lot of noise (almost entirely around the `test_*()` methods) for what feels like very little value.
It looks like there is a [`check_untyped_defs`](https://mypy.readthedocs.io/en/stable/config_file.html#confval-check_untyped_defs) field which should allow us to avoid the noise here. I'm going to try it now.
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 3 comments![]( "Open in Gerrit")
with open('re2.py', 'r') as file:
re2_contents = relativize_native_import(file.read())
with open('re2/__init__.py', 'x') as file:
file.write(re2_contents)```suggestion
with open('re2.py', 'r') as file:
contents = relativize_native_import(file.read())
with open('re2/__init__.py', 'x') as file:
file.write(contents)
```
Done
with open('re2.pyi', 'r') as file:
re2i_contents = relativize_native_import(file.read())
with open('re2/__init__.pyi', 'w') as file:
file.write(re2i_contents)```suggestion
with open('re2.pyi', 'r') as file:
contents = relativize_native_import(file.read())
with open('re2/__init__.pyi', 'x') as file:
file.write(contents)
```
Done
shutil.copyfile('_re2.pyi', 're2/_re2.pyi')```suggestion
shutil.copyfile('_re2.pyi', 're2/_re2.pyi')
with open('re2/py.typed', 'x') as file:
pass
```Done
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 2 comments![]( "Open in Gerrit")
Daniel McClanahanPlease revert `python.yml` and, instead, replace the TODO in `ci-bazel.yml` with something like the following:
```
- name: Test stubs
run: |
DIR=$(mktemp -d)
cp bazel-bin/python/re2_test.runfiles/_main/python/* "${DIR}"
cp python/*.pyi python/mypy.ini "${DIR}"
cd "${DIR}"
mypy .
shell: bash
````python.yml` is a release workflow, not a CI workflow. Moreover, testing the wheels is important for catching any platform-specific breakage before publishing to PyPI, but the stubs aren't going to break like that. (Well, I certainly hope they won't.)
So the reason I wanted to test stubs like that (in the separate dir, along with `re2_test.py`, with the wheel) was to ensure that the specific little magic we've added in `setup.py` (renaming the `.pyi` files and regex replacing to `from . import _re2`) would correctly "import" with mypy when users consume the output wheel.
I totally agree that there's no platform-specific testing to perform here, but in case we decide e.g. to (heaven forfend) add further `.pyi` files in the future (which e.g. may also need renaming and regex replacing imports), I *would* like to have a single test (not platform-specific!) which imports the wheel, and then runs mypy.
What you've written out here is a correct invocation of mypy that corresponds to the way I've been testing locally, but I *also* want to make sure our types work in the mypy context when imported from the wheel.
This is very long-winded (sorry!), but what I'll do is add this "Test stubs" phase you've helpfully written for me, and then I'll try to add a phase that executes mypy when importing types from the wheel (in the CI and not the release workflow).
Done
warn_return_any = True
warn_unused_configs = TrueDaniel McClanahanCan we just set `strict` (or a similar knob) to enable everything and then, if there are problems, squelch them selectively and leave comments to explain what still needs to be fixed?
Working on this! I do not believe there is a `strict` option and instead we need to turn on a million flags \^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^ https://mypy.readthedocs.io/en/stable/config_file.html I will research further!
.........just found the literal `strict` knob. Sorry! Will do!
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 5 comments![]( "Open in Gerrit")
I believe I have figured out how to get some of the type checking to run, although we *won't* get as deep coverage as we would if we painstakingly added type annotations. As maintainer, I absolutely believe you are within your rights to reject the addition `-> None` to every method. However, I want to make it clear that *should* we ever extend the API and its types, we *may* introduce some errors, which might be caught if we added `-> None` to literally every test method in `re2_test.py` as in the first iteration of this change. However, since the re2 python API is explicitly *not* something that is likely to change in the future, I think we are probably safe to avoid that. But I am pretty confident that we *are* missing some "test coverage" of our type annotations by not annotating most of `re2_test.py`. I am totally fine with that if you are.
warn_return_any = True
warn_unused_configs = TrueDaniel McClanahanCan we just set `strict` (or a similar knob) to enable everything and then, if there are problems, squelch them selectively and leave comments to explain what still needs to be fixed?
Daniel McClanahanWorking on this! I do not believe there is a `strict` option and instead we need to turn on a million flags \^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^_\^ https://mypy.readthedocs.io/en/stable/config_file.html I will research further!
.........just found the literal `strict` knob. Sorry! Will do!
Done
I would prefer not to do this; see the comment in `re2_test.py`.
Done
Do we need to add type annotations throughout this file in order to get mypy to do anything useful for us? There seems to be a lot of noise (almost entirely around the `test_*()` methods) for what feels like very little value.
It looks like there is a [`check_untyped_defs`](https://mypy.readthedocs.io/en/stable/config_file.html#confval-check_untyped_defs) field which should allow us to avoid the noise here. I'm going to try it now.
Done
assert match is not NonePlease use `self.assertIsNotNone(…)` here and below.
You're going to hate this, but I'm pretty confident this is literally necessary for mypy, which is able to recognize `x is not None`, but is not able to infer that `self.assertIsNotNone(x)` means `x` is not `None`. With `self.assertIsNotNone(match)`, we get:
```
re2_test.py:172: error: Item "None" of "Match[str] | None" has no attribute "span" [union-attr]
re2_test.py:172: error: Item "None" of "Match[bytes] | None" has no attribute "span" [union-attr]
re2_test.py:172: error: Item "None" of "Match[str] | None" has no attribute "re" [union-attr]
re2_test.py:172: error: Item "None" of "Match[bytes] | None" has no attribute "re" [union-attr]```
Done
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 1 comment![]( "Open in Gerrit")
def generated_package(name):I moved the `os.makedirs('re2')` into a contextmanager in order to clarify some of the hijinx we're performing. I personally think this makes the control flow much more clear, but am totally open to reverting it!
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 1 comment![]( "Open in Gerrit")
def generated_package(name):I moved the `os.makedirs('re2')` into a contextmanager in order to clarify some of the hijinx we're performing. I personally think this makes the control flow much more clear, but am totally open to reverting it!
Moved `shutil.rmtree(name)` out of the `finally` block to avoid delyeeting upon failure!
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 1 comment![]( "Open in Gerrit")
Daniel McClanahanPlease revert `python.yml` and, instead, replace the TODO in `ci-bazel.yml` with something like the following:
```
- name: Test stubs
run: |
DIR=$(mktemp -d)
cp bazel-bin/python/re2_test.runfiles/_main/python/* "${DIR}"
cp python/*.pyi python/mypy.ini "${DIR}"
cd "${DIR}"
mypy .
shell: bash
````python.yml` is a release workflow, not a CI workflow. Moreover, testing the wheels is important for catching any platform-specific breakage before publishing to PyPI, but the stubs aren't going to break like that. (Well, I certainly hope they won't.)
Daniel McClanahanSo the reason I wanted to test stubs like that (in the separate dir, along with `re2_test.py`, with the wheel) was to ensure that the specific little magic we've added in `setup.py` (renaming the `.pyi` files and regex replacing to `from . import _re2`) would correctly "import" with mypy when users consume the output wheel.
I totally agree that there's no platform-specific testing to perform here, but in case we decide e.g. to (heaven forfend) add further `.pyi` files in the future (which e.g. may also need renaming and regex replacing imports), I *would* like to have a single test (not platform-specific!) which imports the wheel, and then runs mypy.
What you've written out here is a correct invocation of mypy that corresponds to the way I've been testing locally, but I *also* want to make sure our types work in the mypy context when imported from the wheel.
This is very long-winded (sorry!), but what I'll do is add this "Test stubs" phase you've helpfully written for me, and then I'll try to add a phase that executes mypy when importing types from the wheel (in the CI and not the release workflow).
Done
Per our offline discussion regarding the desire to avoid building wheels in CI, I have unreverted my changes here, and we now test stubs in every element of all our publish matrices.
I did consider trying to do it just once in the actual publish step, but I didn't want to actually `pip install` any specific wheel before publishing because I felt scared of inducing weird errors, and after publishing is no good. If running `mypy` in every single wheel-building shard takes up a nontrivial amount of time (doubtful), we could perhaps create a venv? We could also try adding one of the wheels to `MYPYPATH` (I think that's how it works) instead of installing.
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 1 comment![]( "Open in Gerrit")
def generated_package(name):Daniel McClanahanI moved the `os.makedirs('re2')` into a contextmanager in order to clarify some of the hijinx we're performing. I personally think this makes the control flow much more clear, but am totally open to reverting it!
Moved `shutil.rmtree(name)` out of the `finally` block to avoid delyeeting upon failure!
Done
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan uploaded new patchset![]( "Open in Gerrit")
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McClanahan (Gerrit)
Daniel McClanahan added 1 comment![]( "Open in Gerrit")
Daniel McClanahanPlease revert `python.yml` and, instead, replace the TODO in `ci-bazel.yml` with something like the following:
```
- name: Test stubs
run: |
DIR=$(mktemp -d)
cp bazel-bin/python/re2_test.runfiles/_main/python/* "${DIR}"
cp python/*.pyi python/mypy.ini "${DIR}"
cd "${DIR}"
mypy .
shell: bash
````python.yml` is a release workflow, not a CI workflow. Moreover, testing the wheels is important for catching any platform-specific breakage before publishing to PyPI, but the stubs aren't going to break like that. (Well, I certainly hope they won't.)
Daniel McClanahanSo the reason I wanted to test stubs like that (in the separate dir, along with `re2_test.py`, with the wheel) was to ensure that the specific little magic we've added in `setup.py` (renaming the `.pyi` files and regex replacing to `from . import _re2`) would correctly "import" with mypy when users consume the output wheel.
I totally agree that there's no platform-specific testing to perform here, but in case we decide e.g. to (heaven forfend) add further `.pyi` files in the future (which e.g. may also need renaming and regex replacing imports), I *would* like to have a single test (not platform-specific!) which imports the wheel, and then runs mypy.
What you've written out here is a correct invocation of mypy that corresponds to the way I've been testing locally, but I *also* want to make sure our types work in the mypy context when imported from the wheel.
This is very long-winded (sorry!), but what I'll do is add this "Test stubs" phase you've helpfully written for me, and then I'll try to add a phase that executes mypy when importing types from the wheel (in the CI and not the release workflow).
Daniel McClanahanDone
Per our offline discussion regarding the desire to avoid building wheels in CI, I have unreverted my changes here, and we now test stubs in every element of all our publish matrices.
I did consider trying to do it just once in the actual publish step, but I didn't want to actually `pip install` any specific wheel before publishing because I felt scared of inducing weird errors, and after publishing is no good. If running `mypy` in every single wheel-building shard takes up a nontrivial amount of time (doubtful), we could perhaps create a venv? We could also try adding one of the wheels to `MYPYPATH` (I think that's how it works) instead of installing.
Done
Related details
- Paul Wankadia
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |