Skip to content

chore(python): remove auto-generated type stubs#7571

Open
DennisOSRM wants to merge 1 commit into
masterfrom
remove_generated_file
Open

chore(python): remove auto-generated type stubs#7571
DennisOSRM wants to merge 1 commit into
masterfrom
remove_generated_file

Conversation

@DennisOSRM
Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings May 21, 2026 06:18
@DennisOSRM
Copy link
Copy Markdown
Collaborator Author

Hey @nilsnolde, the auto-generated stubs should be ok to remove from the repo, right?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Removes a large auto-generated Python type stub file from the repository and prevents it from being re-committed in the future.

Changes:

  • Deleted the auto-generated osrm_ext.pyi stub file.
  • Added a .gitignore rule to ignore src/python/osrm/osrm_ext.pyi.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

File Description
src/python/osrm/osrm_ext.pyi Removed the committed auto-generated type stub.
.gitignore Ignores the generated stub path to keep it out of version control.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nilsnolde
Copy link
Copy Markdown
Contributor

hmmm:

# Generate Python type stubs (.pyi) from the compiled extension module.
# Stubs are written to the source tree so they can be committed and used by
# documentation tools without compiling the extension.

they do have a point, even though API docs might not be a thing yet. but I did want to follow up with a mkdocs PR at some point, to see if I can make it work easily with your current (API) docs and if you'd be open to switch.

should we leave the .pyi stubs for now @DennisOSRM ?

@DennisOSRM
Copy link
Copy Markdown
Collaborator Author

Thanks for the context! My concern with committing the stub is that it creates two sources of truth: the pybind11 bindings and the generated .pyi, and they will drift the moment someone touches the C++ side without regenerating. The gitignore approach keeps things honest.

On the doc tooling point: totally open to a mkdocs PR, but the look and feel of the docs needs to stay on par with what we have in vitepress today. If mkdocs can match that, great. Either way, stub generation is better wired up as a CI step when the pipeline actually exists, rather than committed upfront. We can revisit the gitignore rule as part of that PR.

@nilsnolde
Copy link
Copy Markdown
Contributor

nilsnolde commented May 23, 2026

Thanks for the context! My concern with committing the stub is that it creates two sources of truth: the pybind11 bindings and the generated .pyi, and they will drift the moment someone touches the C++ side without regenerating. The gitignore approach keeps things honest.

there will actually not be any drifting, cmake auto-generates the .pyi files, they're coming from the c++ sources, which is the only source of truth. a CI step already checks if the dev added python api with docstrings and didn't rebuild & commit the output .pyi files. IMO that's a fine approach. sure, a few more big-ish files committed to source, but we never need to touch them.

in fact, I just did the same thing to our valhalla bindings and added a few lines at the top of each .pyi file with a "machine generated" warning, meaning "don't touch" for the dev, we just need to rebuild the bindings locally and commit the output. I can do the same for this repo?

does that sound fair @DennisOSRM ?

@nilsnolde
Copy link
Copy Markdown
Contributor

I do wanna add though that kevin had the same thought as you haha valhalla/valhalla#6101 (comment)

@DennisOSRM
Copy link
Copy Markdown
Collaborator Author

The reason it caught my attention was that the build generated changes to the file. Mostly whitespace (which shall never be discussed). So, there's churn.

@nilsnolde
Copy link
Copy Markdown
Contributor

Urgh, that's annoying, I get it. Maybe we need to pin nanobind to smth specific. Did CI work though? Like it wasn't macos vs linux? Then I can only guess nanobind version..

@nilsnolde
Copy link
Copy Markdown
Contributor

In the end it's your decision. Docs could also be generated in one of the linux jobs dynamically after a build has been done. It's mostly annoying locally when building python API docs, which happens quite rarely. If they add whitespace etc randomly, I'd also be in favor to change it.

@nilsnolde
Copy link
Copy Markdown
Contributor

I can do it. It touches a bit more than this PR, GHA needs to change too.

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