Skip to content

Conversation

@kaushikcfd
Copy link
Collaborator

nanobind_add_stub is a convenient routine, just used that.

@kaushikcfd kaushikcfd marked this pull request as draft February 23, 2025 02:21
@inducer
Copy link
Owner

inducer commented Feb 24, 2025

#150 has some (mostly orthogonal) work. A few of the problems I faced:

  • As far as nanobind is concerned, the wrapper lives in islpy._isl, but that's not a user-exposed thing. So the stubs need to be hacked to disappear the _isl bit.
  • Some functionality actually lives in islpy/__init__.py. Mypy will read from either a stub or a .py, but not both. How can one make types for both available without repeating oneself?

@kaushikcfd
Copy link
Collaborator Author

I don't think there's an issue with the changes to the cmakefile. The proposed solution works locally, i.e. the stubs work as expected. (Linux, py3.13)

I was not able to resolve the issue with the wheels CI for one of the MacOS-13 CI arches.

@kaushikcfd kaushikcfd force-pushed the generate_stubs branch 2 times, most recently from 025a936 to 34c02f6 Compare March 8, 2025 19:04
@kaushikcfd kaushikcfd marked this pull request as ready for review March 8, 2025 22:07
@kaushikcfd
Copy link
Collaborator Author

@inducer: What do you think of this?
For LSPs, this is ok-ish. A problem is the stubgen isn't being precise for the return types, not sure why.

@kaushikcfd kaushikcfd requested a review from inducer March 8, 2025 22:10
@kaushikcfd
Copy link
Collaborator Author

Since #150 also includes some similar bits, feel free to close this and continue with that. One thing I learned during this experiment was that making it work for certain arches of macos inside cibuildwheel was tricky (not possible?), so building the stubs locally and then uploading it made more sense.

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks! A few questions below.

Copy link
Owner

Choose a reason for hiding this comment

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

If this is a generated file, why is it being added to git?

# {{{ Generate stubs (.pyi files)

# Pass -DREBUILD_STUBS to regenerate the stubs.
if(REBUILD_STUBS)
Copy link
Owner

Choose a reason for hiding this comment

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

Why not do this all the time?

Copy link
Owner

Choose a reason for hiding this comment

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

These stubs look a lot better than what my attempt (#150) produced. Yay!

Copy link
Owner

Choose a reason for hiding this comment

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

It seems that this is typing, e.g. islpy._isl.BasicSet. Are the type checkers smart enough to see that as the same type as islpy.BasicSet? What is the canonical name that's shown in error messages? If it's the wrong one, is there a way to fix it? (Could this be called __init__.pyi?)

Copy link
Owner

Choose a reason for hiding this comment

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

__init__.py adds methods to the types from _isl. My understanding is that type checkers will consider either __init__.py or __init__.pyi, but not both. Is there a path for the type checker to know about both the C++ and the Python-implemented bits? One kind-of-hacky way would be to write AST-based code to patch in the stubs from __init__.py.

@inducer inducer mentioned this pull request May 22, 2025
12 tasks
@inducer inducer closed this in #150 Jun 3, 2025
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.

3 participants