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

Basic type-hinting for the pyo3 wrapper. #788

Closed
wants to merge 1 commit into from

Conversation

LLukas22
Copy link
Contributor

@LLukas22 LLukas22 commented Sep 9, 2023

This PR adds some very basic type-hints for the PyO3 wrapper. This should improve the development experience as the python language server can now infer the types and support the user a bit.

BasicTypehinting.mp4

Currently only a few functions (mainly everything used in the test.py) got added to the stub file. If we want to add more type-hints i could add them later.

@@ -101,28 +102,6 @@ impl PyDevice {
}
}

impl<'source> FromPyObject<'source> for PyDevice {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The goal here was to be able to specify the device as a string. Could we maintain this functionality?

@@ -0,0 +1,111 @@
from typing import Any, List, Tuple, Union
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you have to type all this manually? I'm a bit worried that this will be hard to maintain when making changes to the bindings, I imagine that there is no way to generate these automatically instead at the moment? (I would have thought that it's not that hard actually as it's easy to introspect module elements and documentations in python)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i created those manually and you are absolutely right, this will be a nightmare to maintain. I looked into generating them automatically (or at least from the text_signature), but PyO3 doesn't seam to support this currently. See:

This seams to be blocked by CPython directly: python/cpython#101872

I'm think we should close this PR as there will probably be a lot of changes in candles implementation. This is kind of unfortunate as using the python bindings without type-hints can be a bit tedious. Do you have any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you, as you're planning on making changes to the pyo3 bits as long as you keep this file in mind, I'm happy with merging it - especially if you think it makes you more productive when trying out the python bindings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i don't think creating them manually is sustainable in the long run. I probably look into how tokenizers solves this with their stub.py file.

@LLukas22 LLukas22 closed this Sep 10, 2023
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