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 type stubs #46

Merged
merged 3 commits into from
Jul 14, 2021
Merged

Add type stubs #46

merged 3 commits into from
Jul 14, 2021

Conversation

tlambert03
Copy link
Contributor

closes #43 by adding type stubs to the distribution.

I tried to get a good fully-autogenerated stub file, but alas it was beyond me. So this is partially manually created. (I used the Java stubs, with a fair amount of find and replace, and some manual curation). The possibility for errors exists, but I think it's a way better experience than not having them, and it can always be improved with time.

work-in-progress... I still need to test some things locally to make sure that the built package gets recognized in IDE's and such.

@tlambert03 tlambert03 marked this pull request as draft July 14, 2021 00:58
@marktsuchida
Copy link
Member

Hmm. I just remembered that SWIG can (sort of) generate type hints. The generated type hints are strings of C types such as -> "void" instead of -> None, so they are not very useful as is (in fact sometimes harmful in IDEs), but could perhaps serve as a basis for search and replace.

The way to get them is to run SWIG without the -builtin option (but with -py3). This causes the pymmcore/pymmcore_swig.py to contain a proxy class for every wrapped class, and those proxy classes contain the C type hints.

Not sure if this is helpful given that you've already done the work, though.

There is no strong reason for us to be using -builtin, other than that it is faster and I didn't see any downside at the time.

@tlambert03
Copy link
Contributor Author

Still helpful, I've give it a shot. Always better to have something autogenerated. Thanks

@marktsuchida
Copy link
Member

In that case it might make sense to remove -builtin from setup.py, so that we don't need to run SWIG twice in the build. The performance implications are probably not noticeable for something like MMCore (expect ~10 µs per call according to SWIG docs), and I think the behavioral changes are safe for existing user code (unlike the opposite direction of adding -builtin). It will be good to at least casually check that the latter is true (including, say, exceptions thrown), though.

Feel free to add any necessary build dependencies (mypy?).

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jul 14, 2021

thanks. I'm playing with it right now... removing -builtin certainly gives me something, but it still needs a lot of work:

with -builtin

getAllowedPropertyValues(...)
    getAllowedPropertyValues(CMMCore self, char const * label, char const * propName) -> StrVector

    Parameters
    ----------
    label: char const *
    propName: char const *

without -builtin:

getAllowedPropertyValues(self, label: 'char const *', propName: 'char const *') -> 'std::vector< std::string,std::allocator< std::string > >'
    getAllowedPropertyValues(CMMCore self, char const * label, char const * propName) -> StrVector

    Parameters
    ----------
    label: char const *
    propName: char const *

According to the SWIG docs:

In addition, an attempt is made to simplify the type name such that it makes more sense to the Python user. Pointer, reference and const info is removed if the associated type is has an associated Python type (%rename's are thus shown correctly). This works most of the time, otherwise a C/C++ type will be used.

but obviously those annotations aren't going to cut it with mypy. so unless I've missed an obvious setting... I may need to add some typemaps. Happy to keep digging

@marktsuchida
Copy link
Member

Unfortunately I think that is the best you can currently get from SWIG (swig/swig#735), at least without making the .i file harder to maintain.

I was thinking that you might be able to do some AST manipulation on the output of mypy stubgen, to replace the C type strings with Python types. This would avoid running fragile regexes on the code. Also it can fail the build when an unknown string is encountered, so it will be pretty maintainable. I should say I've never actually tried AST manipulation on my own, but it doesn't look that difficult....

@tlambert03
Copy link
Contributor Author

the output of mypy stubgen

yeah, that's more or less how I started with the current file. My suspicion (based on a little experience doing this with other projects) is that it's really hard to get something both thorough and accurate without a little manual work. I'll play with a few more things, but my gut feeling at the moment is that we should just go with the current stub, which we can always update in the future if we hit on a better method. I don't get the sense that the signatures in mmcore are changing that much? (and this has no runtime consequences anyway)

@marktsuchida
Copy link
Member

Sounds good to me. Signatures in MMCore should rarely change (except for adding methods), and in any case should cause a major version increment, so we just need to check when switching MMCore versions.

@tlambert03
Copy link
Contributor Author

ok, I'm giving up on the automation for now ... for the sake of record, here's a script that takes the output of stubgen pymmcore (where pymmcore has been built without -builtin), and converts it to a valid type file.
It's still rather manual (see str2type dict), and still doesn't deal with overloads very well, and also lacks the docstrings (tried to get them from doxygen, but that only seems to look in the .h file, while the good stuff is in the .cpp file).

so, let me just test that the packaged type hints work as is, and then we can iterate on this in later versions if desired

import ast
from ast import Name
import astor
import black

tree = astor.parse_file("out/pymmcore/pymmcore_swig.pyi")

str2type = {
    "char": "str",
    "std::string": "str",
    "unsigned int": "int",
    "unsigned long": "int",
    "double": "float",
    "int": "int",
    "long": "int",
    "bool": "bool",
    "size_t": "int",
    "ptrdiff_t": "int",
    "void": "None",
    "std::map< std::string,std::string >": "Dict[str, str]",
    "StrMap": "Dict[str, str]",
    "std::istringstream": "StringIO",  # probably wrong
    "SwigPyIterator": "Any",
    "PyObject": "Any",
    "swig::SwigPyIterator": "Any",
    # not sure?
    "DoubleVector": "Sequence[float]",
    "LongVector": "Sequence[int]",
    "CharVector": "Sequence[str]",
    "StrVector": "Sequence[str]",
    "std::vector< char >": "Sequence[str]",
    "std::vector< std::string >": "Sequence[str]",
    "std::vector< long >": "Sequence[int]",
    "std::vector< double >": "Sequence[float]",
    "std::vector< unsigned int,std::allocator< unsigned int > >": "Sequence[int]",
    "std::vector< unsigned char *,std::allocator< unsigned char * > >": "Sequence[str]",
    "std::vector< char,std::allocator< char > >": "Sequence=[str]",
    "std::vector< long,std::allocator< long > >": "Sequence[int]",
    "std::vector< double,std::allocator< double > >": "Sequence[int]",
    "std::vector< std::string,std::allocator< std::string > >": "Sequence[str]",
    # internal
    "CMMError::Code": "int",
    "CMMError": "CMMError",
    "MM::DeviceType": "int",
    "MM::PropertyType": "int",
    "MM::DeviceDetectionStatus": "int",
    "PropertySetting": "PropertySetting",
    "Configuration": "Configuration",
    "PropertyPair": "PropertyPair",
    "MMEventCallback": "MMEventCallback",
    "Metadata": "Metadata",
    "MetadataTag": "MetadataTag",
    "MetadataSingleTag": "MetadataSingleTag",
    "MetadataArrayTag": "MetadataArrayTag",
    "PropertyBlock": "PropertyBlock",
}


def _clean_hint(hint):
    hint = hint.replace("const", "").rstrip(" &*")
    if ">::" in hint:
        hint = hint.split(">::")[0] + ">"
    return hint


class V(ast.NodeVisitor):
    def visit_arg(self, node: ast.arg):
        if node.annotation:
            if isinstance(node.annotation, ast.Constant):
                node.annotation = Name(str2type[_clean_hint(node.annotation.value)])
            elif isinstance(node.annotation, ast.Name):
                node.annotation = Name(str2type[_clean_hint(node.annotation.id)])
        return self.generic_visit(node)

    def visit_FunctionDef(self, node: ast.FunctionDef):
        if node.returns:
            if isinstance(node.returns, ast.Name):
                node.returns = Name(str2type[_clean_hint(node.returns.id)])
            elif isinstance(node.returns, ast.Constant):
                if node.returns.value:
                    node.returns = Name(str2type[_clean_hint(node.returns.value)])
        return self.generic_visit(node)



V().visit(tree)
source = "from typing import Dict, Sequence\n"
source += "from io import StringIO\n\n"
source += astor.to_source(tree)
final = black.format_str(source, mode=black.FileMode(is_pyi=True))
with open("stubs.pyi", "w") as f:
    f.write(final)

@tlambert03 tlambert03 marked this pull request as ready for review July 14, 2021 18:48
@tlambert03
Copy link
Contributor Author

ok @marktsuchida, I checked that the stubs get packaged in the wheel, and that mypy and IDEs can see them when the wheel has been installed into a fresh environment. For me, this will be a big quality-of-life improvement as is... but definitely ping me if you run into any issues, or would like to pick up the autogeneration discussion again. I left a comment at the top of the pyi file linking to this issue (and mentioning the script above).

thanks!

@tlambert03 tlambert03 changed the title [WIP] add typing support Add type stubs Jul 14, 2021
@marktsuchida marktsuchida merged commit 3669089 into micro-manager:main Jul 14, 2021
@marktsuchida
Copy link
Member

Thanks!

I actually think your script is pretty good given the constraints we're working with. Maybe I'll take a look at it some time to see if I can resolve some of the question marks.

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.

accept type stubs?
2 participants