Add support for UV & Ruff #38
Conversation
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
| [tool.hatch.build.targets] | ||
| packages = ["kubeflow"] |
There was a problem hiding this comment.
Given that we only have one target, do we need it ?
There was a problem hiding this comment.
I think it was in the file already
https://github.com/kubeflow/sdk/blob/main/python/pyproject.toml#L43-L44
we can remove it if not required
There was a problem hiding this comment.
Yes, we can remove this one.
| path = "kubeflow/trainer/__init__.py" | ||
|
|
||
| [tool.ruff] | ||
| line-length = 100 |
There was a problem hiding this comment.
Does ruff also replace flake8 ?
https://github.com/kubeflow/sdk/blob/4933dae8cd9cad70f1cb5b95903cfe5d09db0e05/.flake8
| [tool.uv] | ||
| package = true |
There was a problem hiding this comment.
since we have [build-system] we can drop this one
.pre-commit-config.yaml
Outdated
| - repo: https://github.com/psf/black | ||
| rev: 25.1.0 | ||
| hooks: | ||
| - id: black |
There was a problem hiding this comment.
Does ruff also replace black and isort ?
There was a problem hiding this comment.
yes, it does replace black and isort
There was a problem hiding this comment.
@szaher In that case, let's remove them from the pre-commit and other configs.
.github/workflows/lint-test.yaml
Outdated
| @@ -0,0 +1,30 @@ | |||
| name: Lint | |||
There was a problem hiding this comment.
I would move this GitHub action under test-python.yaml, with the similar name as here: https://github.com/kubeflow/trainer/blob/master/.github/workflows/test-python.yaml#L1
In that test, we can just run make verify which verifies Python code, and run make test-unit which runs unit tests.
Removing number of parallel GitHub actions will help our CI to execute faster, given that we have limitations for concurrent runs.
.github/workflows/lint-test.yaml
Outdated
| - name: Install uv | ||
| run: pip install uv | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| cd python | ||
| uv sync | ||
| - name: Run ruff | ||
| run: | | ||
| cd python | ||
| uv run ruff check |
There was a problem hiding this comment.
We can move all of this to the Makefile for make verify instruction.
.pre-commit-config.yaml
Outdated
| - repo: https://github.com/psf/black | ||
| rev: 25.1.0 | ||
| hooks: | ||
| - id: black |
There was a problem hiding this comment.
yes, it does replace black and isort
| path = "kubeflow/trainer/__init__.py" | ||
|
|
||
| [tool.ruff] | ||
| line-length = 100 |
| [tool.uv] | ||
| package = true |
There was a problem hiding this comment.
since we have [build-system] we can drop this one
| [tool.hatch.build.targets] | ||
| packages = ["kubeflow"] |
There was a problem hiding this comment.
I think it was in the file already
https://github.com/kubeflow/sdk/blob/main/python/pyproject.toml#L43-L44
we can remove it if not required
|
@szaher Did you get a chance to address the remaining comments, so we can merge this PR ? |
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
.github/workflows/lint-test.yaml
Outdated
| - name: Install bash | ||
| run: sudo apt-get update && sudo apt-get install -y bash |
There was a problem hiding this comment.
bash should be installed on this runner.
We didn't install it here: https://github.com/kubeflow/trainer/blob/master/.github/workflows/test-python.yaml
However, we still use it in the Makefile: https://github.com/kubeflow/trainer/blob/master/Makefile#L11
There was a problem hiding this comment.
I was testing since I was getting errors from .ONESHELL
Makefile
Outdated
| @cd $(PY_DIR) && uvx ruff check --show-fixes | ||
|
|
||
| .PHONY: verify | ||
| verify: verify-lock verify-ruff ## Verify everything is Ok for python sdk |
There was a problem hiding this comment.
Can you just run all instructions under verify to install tools if that is required, similar to this: https://github.com/kubeflow/trainer/blob/master/Makefile#L158
E.g.:
UV := $(shell which uv)
uv:
ifeq ($(UV),)
curl -LsSf https://astral.sh/uv/$(UV_VERSION)/install.sh | sh
$(info uv has been installed)
endif
verify: uv ruff # install all required tools
@cd $(PY_DIR) && uv lock --check
@cd $(PY_DIR) && uvx ruff check --show-fixes
Makefile
Outdated
| UV_VERSION=0.7.2 | ||
| RUFF_VERSION=0.12.3 |
There was a problem hiding this comment.
Is that important which version of uv and ruff we run ?
We can consider to place them under bin/ if specific version is required.
There was a problem hiding this comment.
not really required. I will install the latest
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
.github/workflows/lint-test.yaml
Outdated
| @@ -0,0 +1,26 @@ | |||
| name: Lint | |||
There was a problem hiding this comment.
@szaher Shall we just name this Workflow and rename file to test-python.yaml:
| name: Lint | |
| name: Unit Test - Python |
Similar to Trainer test: https://github.com/kubeflow/trainer/blob/master/.github/workflows/test-python.yaml#L14, we can just run 2 jobs: pre-commit + test
Signed-off-by: Saad Zaher <szaher@redhat.com>
Makefile
Outdated
|
|
||
| PY_DIR := $(PROJECT_DIR)/python | ||
|
|
||
| REPO := github.com/kubeflow/sdk |
There was a problem hiding this comment.
This is unused
| REPO := github.com/kubeflow/sdk |
Makefile
Outdated
| .PHONY: deps | ||
| deps: |
There was a problem hiding this comment.
Can you rename it to uv, it is better to keep instructions with the same name as binaries we install.
| .PHONY: deps | |
| deps: | |
| .PHONY: uv | |
| uv: |
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
|
@andreyvelich one more review please :) |
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
There was a problem hiding this comment.
Thank you for this great contribution @szaher!
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
pyproject.tomlto include directives for uv supportWhich issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #
Checklist: