run ruff on codebase#121
run ruff on codebase#121Mbeaulne wants to merge 1 commit into02-24-adds_ruff_and_mypy_linting_and_type_checkingfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Seems pretty good to me. Can you ask an agent if any of the changes made in the most recent commit (assuming you are checked out on this branch) are breaking changes? I'd be interested to have that peace of mind, especially given my own Python knowledge is limited and I won't be able to properly judge line by line if this is safe. If you think it's safe, and an agent thinks it's safe, and a regression test of the API and orchestrators passes locally, then it's good with me! |
|
Thank you for your work on improving the codebase. There couple of notes:
We use Black for code formatting. Our style guide is based on Google Style guide: https://google.github.io/styleguide/pyguide.html with certain exception like using relative imports.
Note that the type union syntax is https://peps.python.org/pep-0604/ which was only introduced in Python 3.10. Let's make sure the resulting changes work on Python 3.10 minimum version. There are some delayed type annotations that I think are only supported in later Python versions (for example, referring to the function's enclosing class). We also use https://www.conventionalcommits.org/ . This will make it easy to generate changelogs. |
|
|
||
| from kubernetes import client as k8s_client_lib | ||
|
|
||
| from cloud_pipelines.orchestration.storage_providers import google_cloud_storage |
There was a problem hiding this comment.
We usually put cloud_pipelines import in a group adjacent to cloud_pipelines_backend since these are closely related projects.
| request_timeout: int | tuple[int, int] = 10, | ||
| pod_name_prefix: str = "task-pod-", | ||
| gcs_client: "storage.Client | None" = None, | ||
| gcs_client: storage.Client | None = None, |
There was a problem hiding this comment.
In which versions of Python this will work?
cd632fd to
6431a39
Compare
8c96ada to
e72e2ef
Compare
6431a39 to
dec4dc7
Compare
e72e2ef to
8d58045
Compare
dec4dc7 to
3163fa5
Compare
8d58045 to
101fd7b
Compare
morgan-wowk
left a comment
There was a problem hiding this comment.
I left a comment and trust you can do the necessary safety checks before merging / after you have addressed Alexey's comments
101fd7b to
560525e
Compare

TL;DR
Modernized Python code by adopting PEP 585 union syntax, consolidating imports, and applying various code style improvements.
What changed?
typing.Unionandtyping.Optionalwith modern union syntax (|and| None)Optional,OrderedDict, etc.)list,dict,tuple) instead oftypingequivalentsHow to test?
Run the existing test suite to ensure all functionality remains intact. The changes are purely stylistic and should not affect runtime behavior.
Why make this change?
These changes improve code readability and maintainability while adopting modern Python conventions. The new union syntax is more concise and readable, consolidated imports reduce clutter, and removing unused imports helps with code clarity and potentially reduces import overhead.