-
Notifications
You must be signed in to change notification settings - Fork 243
Fix recursive stubgen output structure #1082
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
base: master
Are you sure you want to change the base?
Conversation
Hi @Ahajha -- I am generally open to this change. But I think it would be better to directly generate the desired structure, instead of formulating this as a post-generation cleanup pass. |
@wjakob Makes sense, I had considered that too. The issue I wasn't sure how to solve is that we would need to know ahead of time the tree structure, to know if a certain path can be simplified. This method seemed much more obvious how to implement. I think in order to do this, we would need to count how many submodules we generate in the recursive calls, and then condition the path on that. I'll explore that strategy and get back to you. |
@wjakob I updated with the new logic - it ended up being a bit simpler than post-patching. |
I also included a bit of refactoring since I needed to duplicate some logic between the recursive and base case, including a typing fix: |
This comment was marked as outdated.
This comment was marked as outdated.
ad6ae7f
to
3269314
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@wjakob Can you take another look at this PR? (As well as the other PR to fix the tests?) |
160f9d5
to
df6d659
Compare
bump |
Context here: #989 (comment), but going to add here too.
I'm using https://github.com/mlc-ai/xgrammar as an example here, but I've run into this with an internal codebase as well. I have a PR there to add stubfile generation (https://github.com/mlc-ai/xgrammar/pull/341/files), and I need to work around this issue by fixing up the output tree after building. What happens is that with the
-r
flag, the output tree looks like:This output tree doesn't match the project structure - I would accept one of the two following outputs as valid:
or
The first is more verbose, but valid. The second is a simpler form which I would generally prefer.
This makes the output tree look like the second. Since each module generates all of its submodules before outputting its own stubfile, we can check how many submodules it has - if it's none, we simplify the path.
To be super clear - this is a breaking change, I'm happy to put this behind a flag. Or even two, one for this new structure and one for the tree simplification (though I think the simplification should be the default, and adding a second flag from the getgo is likely premature).
I'm also happy to add tests, just want to make sure that the direction of this PR is okay first before I do that.
Some design notes:
__init__.pyi
would be put in the folder and then tranformed intofoldername.pyi
, which would not be inside the user specified folder.