-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: support recursive stubgen #44
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this up! Here are a few comments.
If I may ask: In which setup do you use recursive stubgen? I'm wondering if for multiple extensions, you need to make all extension labels available to the stubgen binary as data, cf. L198 in the diff.
stubgen_wrapper.py
Outdated
@@ -115,6 +117,16 @@ def wrapper(): | |||
|
|||
main(args) | |||
|
|||
if "-r" in args: | |||
for root, _, filenames in os.walk(runfiles_dir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially very expensive if many packages appear under runfiles, and if they have stubs in them, they might be erroneously copied into $(BINDIR).
I wonder if there is an option for stubgen to put all recursively generated stubs into the same directory? Maybe -O $(BINDIR)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't believe I didn't see that option XD, yeah I'll get it done! Thank you so much
I started a project based off the Nanobind examples bazel branch, but my code was getting a bit chunky so I was looking to modularise the code but I still only wanted a single Nanobind extension. I saw in the Nanobind documentation they have support for sub-modules. I was hoping ultimately have source code looking something like:
And produce a wheel that looks something like:
But my MR is not entirely correct with naming the stubs correctly, at the moment |
I had another go, I also have modified the Bazel Nanobind example which can be found here for a minimal example. Unfortunately the root stub is named like If you use this branch with the this example branch, you should get a working (brittle) example. |
stubgen_wrapper.py
Outdated
if "-O" in args: | ||
# TODO: Not sure what is the best way to handle this, using the Nanobind Bazel example, | ||
# the root stub looks like _main.src.nanobind_example.pyi... | ||
shutil.move(Path(bindir) / f'{output_dir}/_main.{output_dir.replace("/", ".")}.pyi', Path(bindir) / f'{output_dir}/__init__.pyi') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember encountering this when attempting to write stubs relative to the working directory in a Bazel project - I think you cannot avoid botched stub names when specifying the output directory and it's not the same as the current working directory.
What I would like is this:
src/nanobind_example/__init__.py
src/nanobind_example/nanobind_example_ext.pyi
src/nanobind_example/sub_ext/__init__.py
src/nanobind_example/sub_ext/__init__.pyi # <- this would contain stubs for sub()
I'll still have to check what stubs are actually produced with your changes, but tl,dr: I think the correct solution is that top-level module extensions get named stubs, and submodules get __init__.pyi
files.
Maybe we can avoid the move hack by tweaking some logic in nanobind.stubgen, I'll check it out.
stubgen_wrapper.py
Outdated
# we have an output directory, use its path instead relative to $(BINDIR), | ||
# but in absolute form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this comment? Since the output dir is relative to BINDIR, it cannot be absolute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was copying a similar comment that can be found on line 108, but have now removed it
Hi @cemlyn007, how is the situation? (I can also take over from here if you like.) |
Hey @nicholasjng, sorry for the delay, I've made some changes based on your comments, I do still feel like the top-level stub renaming is hacky but I'm not sure how we can do it in a better way. FYI for testing I was pip installing and inspecting the file structure in site packages, but this wasn't great because for reasons unknown to me that file structure looked dirty (had old files being seen in). I thought a simple I think for testing this PR, one working way is to use the python
If you have a better strategy for testing I am all ears, also let me know if you would like any further changes |
Hey! I merged your additions in the nanobind_example. Could you rebase on current master, so that the CI changes are reflected here? |
To support producing stubs for submodules.
#43