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 python type information for MessageQueue class #54

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

sillydan1
Copy link

@sillydan1 sillydan1 commented Sep 21, 2024

Note that this only includes the MessageQueue class as it was the only thing I needed (so far) - I would love to add the rest of the python bindings as well if this is a desired addition.

Added the rest of the things mentioned in USAGE.md for the sake of completeness.

@osvenskan
Copy link
Owner

Hi @sillydan1, thanks for the PR! Naïve question -- what tools exist for me to test that the info in posix_ipc.pyi matches the implemented module? I trust your work, it's my own I'm worried about. 🙂 When I modify the module in the future, I don't trust myself to keep posix_ipc.pyi accurate.

@sillydan1
Copy link
Author

sillydan1 commented Sep 26, 2024

Hi @sillydan1, thanks for the PR! Naïve question -- what tools exist for me to test that the info in posix_ipc.pyi matches the implemented module? I trust your work, it's my own I'm worried about. 🙂 When I modify the module in the future, I don't trust myself to keep posix_ipc.pyi accurate.

I just cross-referenced the posix_ipc_module.c and USAGE.md files 🤷 .

After a bit of googling, I found that you could use pybind11-stubgen to generate these files directly.
The only problem is that it also adds values to constants and it can't deduce function argument types, so that still have to be done manually.

I've done some usage exploration and it there are still some things that are missing:

  • It seems that the type info files are not being included in the .whl packaging yet.
  • pybind11-stubgen adds a bit more info that what I had made by hand - I will add this info.

@osvenskan
Copy link
Owner

I just cross-referenced the posix_ipc_module.c and USAGE.md files 🤷 .

Sure, that makes sense, and I trust you, but it's not an automated process. This module is extremely stable, so I'm not too worried that it will require a lot of work to keep posix_ipc.pyi accurate. But I don't use typing myself, so I probably won't think about posix_ipc.pyi too much, and if I break it by changing the module interface, I might not notice. I want to be able to run tests that assert that posix_ipc.pyi is correct. "Correct" in this case means "matches the module" and "is complete".

For that, I would need something that can consume posix_ipc.pyi and test that the interface of runtime module matches the information read from posix_ipc.pyi. For instance, I want an automated test to assert that --

  • Everything in posix_ipc.pyi exists in the posix_ipc module
  • Everything in the module is described in posix_ipc.pyi
  • The provided types are correct.

I realize this last point would require that I write some custom test code to execute functions and examine the type of the returned object, but the first two items feel like fairly straightforward introspection and comparison. I feel like every project that provides a .pyi must have a similar need. Is there a tool for that?

@sillydan1
Copy link
Author

sillydan1 commented Sep 27, 2024

Is there a tool for that?

I am not aware of a standalone tool that can do that, but it should be possible to write one based on the output generated from pybind11-stubgen 🤔 I might give it a try some time next week

@osvenskan
Copy link
Owner

OK, thanks. What tool do you use to consume the info in posix_ipc.pyi?

@sillydan1
Copy link
Author

OK, thanks. What tool do you use to consume the info in posix_ipc.pyi?

Just my text editor's language server, which is using pyright, basedpyright, or mypy behind the scenes.

@sillydan1
Copy link
Author

I wrote a draft of what a type-test would look like. It compares the pybind11-stubgen generated types with the hand-crafted ones. However, the test fails because of sizmailov/pybind11-stubgen#231 and it can't test the types of the function parameters.

This is quite the rabbit hole and possibly not worth the effort. I think I might just use the package untyped and abandon this mini-adventure / side-quest.

@@ -9,5 +9,5 @@ matrix:
osx_image: xcode11.2 # Python 3.7
language: shell # 'language: python' is an error on Travis CI macOS

install: python setup.py install
install: python setup.py install && python pip install pybind11-stubgen
Copy link
Author

Choose a reason for hiding this comment

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

Note: I haven't used travis CI before, so I dont know if this is the correct place to do this.

@osvenskan
Copy link
Owner

This is quite the rabbit hole and possibly not worth the effort. I think I might just use the package untyped and abandon this mini-adventure / side-quest.

I appreciate the well-intentioned PR, and I you for talking through it with me. I apologize for my lack of knowledge about Python typing, the state of tooling, best practices, etc. Here's some thinking out loud, using mypy as a proxy for all type checkers since it seems to be the most popular. I welcome your comments and thoughts. I apologize if any of this is blindingly obvious.

The three criteria I mentioned above are what I'd want to satisfy in order to feel confident about the stub file.

  1. Everything in the stub file exists in the posix_ipc module
  2. Everything in the module is described in stub file
  3. The types described in the stub file are correct

I think mypy's stubtest feature would handle item 1. The feature has caveats, but its runtime introspection should raise an alert if it can't find something in the module that it expects to be there.

Item 2 is more difficult, AFAICT. It sounds like stubgen (https://mypy.readthedocs.io/en/stable/stubgen.html#stubgen) can create the stub file, but the doc says "The auto-generated stub files often require some manual updates...". It sounds like it's up to the module owner (me) to fill in the details and get them right, which opens up a new space for bugs to occur. That might be an acceptable tradeoff, though.

Item 3 can probably be covered with a mix of mypy and custom tests. I'm sure mypy can handle the simple stuff like "check that QUEUE_PRIORITY_MAX is in the range of an int" but I will have to write (or add to) existing tests to ensure that e.g. Semaphore.acquire() really returns None. This is where I'm not clear on what support I can get from mypy or a similar tool. It's promising that mypy can read stub files, but can I use mypy to selectively extract info from the stub file during a test? For instance, can I call some mypy function to tell me what the stub file defines as the return type of Semaphore.acquire()?

Even though I don't use typing, I understand that it's valuable to others so I'm willing to support it. I want to make sure I get it right, though. I'm generally of the mind that vague and correct is better than specific and wrong, and that certainly applies here.

@sillydan1
Copy link
Author

sillydan1 commented Oct 6, 2024

That sounds more or less correct.

For instance, can I call some mypy function to tell me what the stub file defines as the return type of Semaphore.acquire()?

In terms of asserting the function parameters and return types, I would recommend using the inspect module (something like this) You could take the approach that I did with using unit tests, where you import ... as stub_posix_ipc and then compare the two modules as thoroughly as possible.

Item 2 is more difficult, AFAICT. It sounds like stubgen (https://mypy.readthedocs.io/en/stable/stubgen.html#stubgen) can create the stub file, but the doc says "The auto-generated stub files often require some manual updates...". It sounds like it's up to the module owner (me) to fill in the details and get them right, which opens up a new space for bugs to occur. That might be an acceptable tradeoff, though.

I was thinking that maybe just autogenerating the stub file using either the pybind11-stubgen (assuming they fix the sorting issue that I linked) or mypy's stubgen as you mentioned, would probably be the most reliable method of integrating types. It would also be immune to implementation error on your end - even though it might not be complete.
In regards to ".. new space for bugs to occur" - python typing are just hints anyway, so if you mess up a type annotation it's not going to be the end of the world - it's just slightly misguiding the user and they would be able to change the stub-file via a PR if they find it too annoying.

As for me, I'll just use the module untyped, as this was a side-quest for a side-project to my side-project. I think I'll just "nip it in the bud" and move along 😄 hope that is okay

@osvenskan
Copy link
Owner

In terms of asserting the function parameters and return types, I would recommend using the inspect module (something like this) You could take the approach that I did with using unit tests, where you import ... as stub_posix_ipc and then compare the two modules as thoroughly as possible.

Thanks. I looked at your unit tests, and that filled in the missing link for me. I didn't realize that the .pyi file was readable by Python. Now that I understand that I can import it directly, I understand how to extract information from it. That's easier than I thought. I thought I would need a third-party tool to do it.

I was thinking that maybe just autogenerating the stub file using either the pybind11-stubgen (assuming they fix the sorting issue that I linked) or mypy's stubgen as you mentioned, would probably be the most reliable method of integrating types. It would also be immune to implementation error on your end - even though it might not be complete. In regards to ".. new space for bugs to occur" - python typing are just hints anyway, so if you mess up a type annotation it's not going to be the end of the world - it's just slightly misguiding the user and they would be able to change the stub-file via a PR if they find it too annoying.

This is fascinating to me 🙂 One of the reasons I haven't been very interested in typing in Python is precisely because they're only hints and not guaranteed to be correct. To me, the knowledge that "f() probably returns an int" feels more distracting than helpful. You feel quite differently, I take it? I'm not against supporting this feature, I'm just trying to wrap my head around it.

As for me, I'll just use the module untyped, as this was a side-quest for a side-project to my side-project. I think I'll just "nip it in the bud" and move along 😄 hope that is okay

That's totally fine and I appreciate all the input and effort. Please leave this pull request open, because I don't want my preferences to get in the way of a feature that other people might find useful. I'd like more time to consider it.

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.

2 participants