-
Notifications
You must be signed in to change notification settings - Fork 86
chore: upgrade code style for python3.10 #288
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: main
Are you sure you want to change the base?
chore: upgrade code style for python3.10 #288
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This disables a couple ruff rules in pyproject.toml: ``` "UP007", # Use X | Y instead of Union[X, Y] (requires Python 3.10+) "UP045", # Use X | None instead of Optional[X] (requires Python 3.10+) ``` Then the code changes are made with: ``` uv run ruff check --fix uv run ruff format ``` Signed-off-by: Jon Burdo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates the codebase to use Python 3.10+ type-hint syntax (PEP 604 unions) and enhances the Katib/Kubernetes optimizer utility to better handle Optional[T] vs T | None type metadata when converting string values.
Changes:
- Enable Ruff/pyupgrade enforcement of
T | UandT | Noneby removing the ignores for UP007/UP045. - Convert many public and internal type hints from
Optional[...]/Union[...]to PEP 604|syntax across trainer/optimizer/common modules. - Update
convert_valueto unwrap union-with-None types and add a targeted unit test.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Removes UP007/UP045 ignores so Ruff enforces modern union/optional syntax. |
| kubeflow/trainer/types/types.py | Rewrites many dataclass field annotations to `T |
| kubeflow/trainer/test/common.py | Updates shared test utility dataclass annotations to PEP 604 style. |
| kubeflow/trainer/options/kubernetes.py | Updates option call signatures/types to PEP 604 unions. |
| kubeflow/trainer/options/common.py | Updates common option call signatures/types to PEP 604 unions. |
| kubeflow/trainer/backends/localprocess/utils.py | Updates helper function annotations to PEP 604 optionals. |
| kubeflow/trainer/backends/localprocess/types.py | Updates localprocess model annotations to PEP 604 optionals. |
| kubeflow/trainer/backends/localprocess/job.py | Updates command parameter union annotation to PEP 604 style. |
| kubeflow/trainer/backends/localprocess/backend.py | Updates backend API method annotations to PEP 604 unions/optionals. |
| kubeflow/trainer/backends/kubernetes/utils.py | Updates Kubernetes backend utility function annotations to PEP 604 style. |
| kubeflow/trainer/backends/kubernetes/backend_test.py | Updates test helpers’ annotations to PEP 604 style. |
| kubeflow/trainer/backends/kubernetes/backend.py | Updates Kubernetes backend API method annotations to PEP 604 unions/optionals. |
| kubeflow/trainer/backends/container/types.py | Updates container backend config model annotations to PEP 604 optionals. |
| kubeflow/trainer/backends/container/runtime_loader.py | Updates runtime loader helper return types to PEP 604 optionals. |
| kubeflow/trainer/backends/container/backend_test.py | Updates test adapter/type annotations to PEP 604 optionals/unions. |
| kubeflow/trainer/backends/container/backend.py | Updates container backend API method annotations to PEP 604 unions/optionals. |
| kubeflow/trainer/backends/container/adapters/podman.py | Updates adapter method signatures to PEP 604 optionals. |
| kubeflow/trainer/backends/container/adapters/docker.py | Updates adapter method signatures to PEP 604 optionals. |
| kubeflow/trainer/backends/container/adapters/base.py | Updates abstract adapter signatures to PEP 604 optionals. |
| kubeflow/trainer/backends/base.py | Updates base backend interface annotations to PEP 604 unions/optionals. |
| kubeflow/trainer/api/trainer_client.py | Updates client API annotations to PEP 604 unions/optionals. |
| kubeflow/optimizer/types/search_types.py | Updates search space type hints to PEP 604 unions. |
| kubeflow/optimizer/types/optimization_types.py | Updates optimization types to PEP 604 unions/optionals. |
| kubeflow/optimizer/types/algorithm_types.py | Updates algorithm types to PEP 604 optionals. |
| kubeflow/optimizer/backends/kubernetes/utils_test.py | Adds unit tests covering union/optional handling in convert_value. |
| kubeflow/optimizer/backends/kubernetes/utils.py | Improves convert_value union-with-None handling; updates return type hints elsewhere. |
| kubeflow/optimizer/backends/kubernetes/backend.py | Updates optimizer backend method annotations to PEP 604 optionals. |
| kubeflow/optimizer/backends/base.py | Updates optimizer base interface annotations to PEP 604 optionals. |
| kubeflow/optimizer/api/optimizer_client.py | Updates optimizer client API annotations to PEP 604 optionals. |
| kubeflow/common/utils.py | Updates common helper signature annotations to PEP 604 optionals. |
| kubeflow/common/types.py | Updates shared config model annotations to PEP 604 optionals. |
Comments suppressed due to low confidence (2)
kubeflow/trainer/backends/container/adapters/podman.py:214
list_containersacceptsfilters: ... | None = Nonebut immediately callsfilters.items(), which will raise whenfiltersis omitted/None; either makefiltersrequired (no default) or guard against None (e.g., treat None as an empty dict).
def list_containers(self, filters: dict[str, list[str]] | None = None) -> list[dict]:
"""List Podman containers with optional filters."""
# Work-around for https://github.com/containers/podman-py/issues/542
for k, v in filters.items():
if len(v) == 1:
kubeflow/trainer/backends/kubernetes/backend_test.py:226
pip_index_urlsis annotated aslist[str] | Nonebut the function unconditionally indexes/slices it, so passingNonewill raise; either drop| Nonefrom the type or handleNoneby falling back toconstants.DEFAULT_PIP_INDEX_URLSinside the function.
def get_custom_trainer(
env: list[models.IoK8sApiCoreV1EnvVar] | None = None,
pip_index_urls: list[str] | None = constants.DEFAULT_PIP_INDEX_URLS,
packages_to_install: list[str] = ["torch", "numpy"],
image: str | None = None,
) -> models.TrainerV1alpha1Trainer:
"""
Get the custom trainer for the TrainJob.
"""
pip_command = [f"--index-url {pip_index_urls[0]}"]
pip_command.extend([f"--extra-index-url {repo}" for repo in pip_index_urls[1:]])
| if target_type is int: | ||
| return int(raw_value) |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In convert_value, the boolean parsing lowercases the input but compares against a capitalized token ("True"), so values like "True"/"true" will parse as False; compare against lowercase tokens (e.g., "true") and consider adding a test case for "true"/"false" in addition to "1"/"0".
The convert_value function didn't seems to be handling union types properly and also needs to handle `T | None` similarly to `Optional[None]` after the upgrade to Python 3.10. Signed-off-by: Jon Burdo <[email protected]>
61ec351 to
01a13b0
Compare
Pull Request Test Coverage Report for Build 21925778716Details
💛 - Coveralls |
| "UP007", # Use X | Y instead of Union[X, Y] (requires Python 3.10+) | ||
| "UP045", # Use X | None instead of Optional[X] (requires Python 3.10+) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first commit I just removed these and ran:
uv run ruff check --fix
uv run ruff format
| if origin is Optional: | ||
| target_type = args[0] | ||
| # `T | None` produces UnionType instead of Union before Python 3.14. | ||
| # `Optional[T]` always produces Union | ||
| if origin is Union or origin is UnionType: | ||
| args = get_args(target_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_origin isn't perfectly consistent across python versions
$ python3.13 - <<'EOF'
from typing import get_origin, Optional
print(get_origin(int | None))
print(get_origin(Optional[int]))
EOF
<class 'types.UnionType'>
typing.Union
$ python3.14 - <<'EOF'
from typing import get_origin, Optional
print(get_origin(int | None))
print(get_origin(Optional[int]))
EOF
<class 'typing.Union'>
<class 'typing.Union'>
astefanutti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great, thanks @jonburdo!
/lgtm
What this PR does / why we need it:
Primarily updates code style after upgrading the min Python version to 3.10 to change;
Union[T, U]→T | UOptional[T]→T | Noneas suggested here: #282 (comment)
Also includes a second commit to handle such union types in
convert_valueutility with a test.Checklist: