Skip to content

Add pyrefly config for smart_control - #175

Open
TakshPatel5821 wants to merge 3 commits into
google:copybara_pushfrom
TakshPatel5821:patch-1
Open

Add pyrefly config for smart_control#175
TakshPatel5821 wants to merge 3 commits into
google:copybara_pushfrom
TakshPatel5821:patch-1

Conversation

@TakshPatel5821

Copy link
Copy Markdown

Addresses #174.

Internally at Google, pyrefly has replaced pylint, and this issue asked for the repo's tooling to be updated to match. This PR adds the "blessed" pyrefly.toml from the issue, scoped to project-includes = ["smart_control"] (mirroring the files: '^smart_control/' pattern already used for the pyink/isort/pylint pre-commit hooks) and excluding the generated *_pb2.py files and smart_control/proto/, the same way isort and the pylint hook already do.

pyrefly is also added to the dev dependency group in pyproject.toml so it installs alongside pylint via poetry install --with dev.

What I intentionally left out:

  • poetry.lock: adding the dependency requires a full poetry lock regeneration (~300 line diff of hashes), which isn't safe for me to hand-edit and didn't want to guess at from outside a real dev environment. Running poetry lock locally should be a one-line fix if a maintainer wants to land this as-is.
  • Wiring pyrefly into .pre-commit-config.yaml as a blocking hook: I ran pyrefly check --config pyrefly.toml against the repo and it currently surfaces 600+ pre-existing type errors under smart_control/ (mostly missing-import noise from optional deps, but also some real bad-argument-type findings). Turning this on as a blocking hook today would break every contributor's pre-commit run for unrelated changes. Happy to add the hook in this PR too if you'd rather land it non-blocking to start, or as a separate PR once there's a plan for the existing errors.

Let me know which direction you'd prefer and I can adjust.

Add initial configuration for Pyrefly with settings for Python version, project includes/excludes, and error handling.
@google-cla

google-cla Bot commented Aug 5, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@s2t2

s2t2 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Hi @TakshPatel5821 welcome! Thanks for your contribution, and for your notes.

  1. If you are able to easily create and share a Google sheet containing the current errors being thrown, it would help me understand the different categories that need to be addressed, and the extent of the updates that need to be made. Otherwise no worries, as many of these errors should hopefully be addressed in an upcoming code sync out (see notes below).

  2. It looks like we have some failing CI checks regarding the poetry.lock file. I think we do want to auto-generate a new lock file and check it in

https://github.com/google/sbsim/actions/runs/31047696462/job/92663756344?pr=175

pyproject.toml changed significantly since poetry.lock was last generated. Run poetry lock to fix the lock file.

  1. Ideally we would integrate pyrefly into our GitHub Actions workflow and pre-commit hooks. Please let me know if you are up to taking this on.

Finally, as some background info, we are preparing a big sync-out of some internal code - that code should hopefully be passing all the pyrefly rules, so it will likely not be necessary to address / fix the current pyrefly issues separately on the GitHub side. To coordinate with this upcoming sync-out, we might sync-out first and then merge the pyrefly updates. I need to think about which should come first. Perhaps we merge the pyrefly config first and ignore existing warnings.

@TakshPatel5821

TakshPatel5821 commented Aug 6, 2026

Copy link
Copy Markdown
Author

Thanks for the response!

  1. Put the breakdown in a sheet instead of a wall of text here: https://docs.google.com/spreadsheets/d/1K9d8YjR-9PxajfePIp-ppJmDGfYRkQFsEC_X8Xu9QAk/edit

most of it is really one root cause — 194 of the 656 errors are proto message classes used in type hints (e.g. Sequence[smart_control_normalization_pb2.ContinuousVariableInfo]), pyrefly just can't resolve GeneratedProtocolMessageType as a valid type form. Known limitation with protoc-generated stubs, not really fixable on the smart_control side. The rest is spread pretty thin across ~170 file/category combinations, nothing else stands out as a single big blocker.

Also noticed project-includes = ["smart_control"] pulls in the demo notebooks too (TD3/SAC/DDPG/CQL_Demo.ipynb, 16 errors each). Let me know if you'd rather exclude those.

  1. poetry.lock is pushed. Ran poetry lock for real this time, then double checked it against Poetry 2.1.2 specifically since that's what your CI installs — installs clean with pyrefly and everything else.

  2. Happy to do the pre-commit/CI wiring, but since the sync-out is coming I'd lean toward waiting so we're not fixing the same warnings twice. Your call though, can do it non-blocking now if you'd rather not wait on that.

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