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

Incorrect python typehint for add_items #23

Open
markkohdev opened this issue Sep 20, 2023 · 4 comments
Open

Incorrect python typehint for add_items #23

markkohdev opened this issue Sep 20, 2023 · 4 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@markkohdev
Copy link
Contributor

The python typehint for add_items (at least in IntelliJ) incorrectly names the second parameter numpy_float32 instead of ids
image

I'm guessing this is due to something wonky in either our bindings or in pybind11. I also noticed that there's a tiny typo in the docstring for the method -- id -> ids
https://github.com/spotify/voyager/blob/main/python/bindings.cpp#L433C7-L433C7

@markkohdev markkohdev self-assigned this Sep 20, 2023
@markkohdev markkohdev added bug Something isn't working documentation Improvements or additions to documentation labels Sep 20, 2023
@markkohdev
Copy link
Contributor Author

Looks like this happens with query() as well
image

@eXamadeus
Copy link

eXamadeus commented Jan 1, 2024

UPDATE: My comment below is definitely not related, but some others may come along and see it, seeing the same issue I did, so I'll leave it here.

I don't know if this is related, but when you use a static method (like load), it also complains since it thinks that the first argument should be self even though the load method and certain other functions are, in fact, statically available on the class.

Ex:

from voyager import Index

loaded = Index.load(f"indexes/{path}.voy")
image

It seems to only have type hints for instances. Then again, I haven't written production code for Python in years (before type hints were ubiquitous), so I might just have something configured poorly.

image

EDIT: I dug through the source code and found where the load method is being bound statically. Definitely seems like a totally unrelated issue, so sorry for posting it here. Anyway, for those curious, here is the definition for the load method:

voyager/python/bindings.cpp

Lines 970 to 978 in f780757

index.def_static(
"load",
[](const std::string filename) -> std::shared_ptr<Index> {
py::gil_scoped_release release;
return loadTypedIndexFromStream(
std::make_shared<FileInputStream>(filename));
},
py::arg("filename"), LOAD_DOCSTRING);

I am not sure why def_static produces the following for the Python stub:

    def load(self, *args, **kwargs): # real signature unknown; restored from __doc__

I would assume it would look like this, but I'm completely unfamiliar with pybind11.

    @staticmethod
    def load(*args, **kwargs): # real signature unknown; restored from __doc__

@markkohdev
Copy link
Contributor Author

@eXamadeus thanks for highlighting this! Your issue isn't exactly the same thing, but it's close enough that I can try to fix both of these at once when I'm done with my current improvement.

For context, since we use python bindings written in c++ to call out to the core C++ code, we actually have to generate these typehints based on the c++ binding code. We do that with this script and so somewhere in the process the staticmethod indicator likely isn't getting acknowledged. The binding code itself does contain an indicator that the method is static so we should be able to populate that accordingly.

@eXamadeus
Copy link

Yeah what I noticed definitely isn't directly related, at best it is ancillary since it relates to the bindings. Either way, thank you for the context.

I'll poke around in my freetime to see if I can help. I can't promise much, since it's been even longer since I've written any C++, haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants