Skip to content

fix(py): propagate rules_python type stubs - #1523

Open
mkanat wants to merge 1 commit into
aspect-build:mainfrom
mkanat:fix/propagate-rules-python-pyi
Open

mkanat wants to merge 1 commit into
aspect-build:mainfrom
mkanat:fix/propagate-rules-python-pyi

Conversation

@mkanat

@mkanat mkanat commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This is the most conservative change possible to make protobuf pyi files (which use rules_python underneath) propagate correctly (see my comment in #558). I could have made all the rules_py rules support putting pyi files in srcs, or made a new pyi_srcs, but I didn't want to propose a new API right now, so this seemed safer.

I did not just slop this PR out, I did review everything and have done multiple refinements to it on top of Codex's output. However, I'm not deeply familiar with all of the rules_py codebase so it's possible some of this isn't structured ideally.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 8764910e-3607-4d20-8502-a3da5c328fb6

📥 Commits

Reviewing files that changed from the base of the PR and between 3609650 and d612b53.

📒 Files selected for processing (14)
  • docs/api/py.md
  • py/private/py_info.bzl
  • py/private/py_info_interop.bzl
  • py/private/py_library.bzl
  • py/private/py_unpacked_wheel.bzl
  • py/private/py_venv/py_venv.bzl
  • py/private/py_venv/py_venv_exec.bzl
  • py/private/py_venv/types.bzl
  • py/tests/py-info-interop/BUILD.bazel
  • py/tests/py-info-interop/library.pyi
  • py/tests/py-info-interop/pyi_propagation_test.bzl
  • py/tests/py_venv_conflict/collision_order_test.bzl
  • py/tests/py_venv_conflict/site_merge_order_test.bzl
  • uv/private/whl_install/rule.bzl

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

Changes

Transitive Python type stub propagation

Layer / File(s) Summary
Provider contracts and interoperability
py/private/py_info.bzl, py/private/py_info_interop.bzl, docs/api/py.md
Providers and documentation define transitive_pyi_files. Interoperability reads the field from either provider or returns an empty depset.
py_library propagation and validation
py/private/py_library.bzl, py/tests/py-info-interop/*
py_library collects .pyi files from dependencies and resolved virtual dependencies. Analysis tests verify cross-provider propagation.
Virtualenv and provider integration
py/private/py_venv/*, py/private/py_unpacked_wheel.bzl, py/tests/py_venv_conflict/*, uv/private/whl_install/rule.bzl
Virtualenv providers carry .pyi files. Wheel and conflict fixtures initialize the field explicitly.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to d612b

This change propagates Python type-stub metadata through providers and virtualenv integration without identified merge-blocking risk.

Sequence Diagram(s)

sequenceDiagram
  participant rules_python_library
  participant py_library
  participant PyInfo
  participant pyi_propagation_test
  rules_python_library->>py_library: Provide library.pyi through rules_python PyInfo
  py_library->>PyInfo: Publish transitive_pyi_files
  PyInfo->>pyi_propagation_test: Expose propagated library.pyi
  pyi_propagation_test-->>py_library: Assert .pyi propagation and empty sources
Loading

Suggested reviewers: jbedard

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 13 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely states the main change: propagation of rules_python type stubs.
Description check ✅ Passed The description directly explains the purpose of the changes and the conservative implementation approach for propagating protobuf .pyi files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 13.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 13 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jbedard

jbedard commented Sep 8, 2026

Copy link
Copy Markdown
Member

@codex

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-08T21:17:25.709281Z d612b53 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@jbedard

jbedard commented Sep 8, 2026

Copy link
Copy Markdown
Member

@mkanat how can we test this? Checkout the existing typecheck CI job, but maybe you have other ideas?

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d612b530a3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread py/private/py_info_interop.bzl
@xangcastle

Copy link
Copy Markdown
Member

Thanks @mkanat. I picked up your commit as-is (authorship kept) in #1538 and built the rest on top: .pyi files in srcs are partitioned into transitive_pyi_files, venvs and launchers carry stubs into runfiles, and there is analysis + e2e coverage (rules_python pyi_srcs, protobuf _pb2.pyi, the compat flag). #1538 supersedes this one; I'll close it once that lands.

@mkanat

mkanat commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Okay, thanks! Sorry I wasn't able to respond in time here, just been really busy.

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