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

feat: support recursive stubgen #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cemlyn007
Copy link

To support producing stubs for submodules.

#43

Copy link
Owner

@nicholasjng nicholasjng left a 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.

Comment on lines +202 to +204
if recursive:
args.append("-r")
Copy link
Owner

Choose a reason for hiding this comment

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

I think we might need some logic to ensure that -o <filename> (the output file, which is optional) and -r do not appear in the same stubgen call.

Could you try if something like this would work:

if recursive and output_file is not None:
    fail("Cannot specify an output file if recursive stubgen is requested")

Alternatively, we could let the stubgen script itself throw the exception, but then we should make a note of this in the docstring. Do you have a preference?

Copy link
Author

Choose a reason for hiding this comment

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

Personally don't mind, I'll add the check in the Bazel side

Comment on lines 88 to 90
if "-r" in args:
pass
elif "-o" not in args:
Copy link
Owner

Choose a reason for hiding this comment

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

This is to work around the recursive + output_file case, right? We should catch that earlier, see my above comment.

@@ -115,6 +117,16 @@ def wrapper():

main(args)

if "-r" in args:
for root, _, filenames in os.walk(runfiles_dir):
Copy link
Owner

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)?

Copy link
Author

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

@cemlyn007
Copy link
Author

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.

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:

src/packageA/moduleA_0/<some_python_files>
src/packageA/ext_moduleA_0/ext_moduleA_0.cpp <- The Nanobind extension
src/packageA/ext_moduleA_0/sub_module_A_0_a/<some_cpp_files_included_by_ext_moduleA_0.cpp>
src/BUILD.bazel

And produce a wheel that looks something like:

packageA/moduleA_0/<some_python_files>
packageA/moduleA_0/ext_moduleA_0.so
packageA/moduleA_0/__init__.pyi
packageA/moduleA_0/sub_module_A_0_a/__init__.py

But my MR is not entirely correct with naming the stubs correctly, at the moment packageA/moduleA_0/__init__.pyi looks like packageA/moduleA_0/_main.src.__init__.pyi (I think but AWOL from my home desktop), and they're not being picked into the wheel, although that might be resolved when I try your comment with using the -O flag in the stubgen script 🤷🏻‍♂️

@cemlyn007
Copy link
Author

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 _main.src.pyi which led me to do some awful hackery, would appreciate any advise on how to better tackle it!

If you use this branch with the this example branch, you should get a working (brittle) example.

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')
Copy link
Owner

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.

Comment on lines +114 to +115
# we have an output directory, use its path instead relative to $(BINDIR),
# but in absolute form.
Copy link
Owner

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.

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.

2 participants