-
Notifications
You must be signed in to change notification settings - Fork 41
chore: Update CONTRIBUTING.md to use uv #41
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
Changes from 25 commits
6eb90f4
665f199
c91307e
bffa122
1c1c886
1998f0b
760ae1a
8d88050
6c4dd97
ec4ad5b
d391711
0783b83
1a38742
7f3458e
e00b711
4a02853
958d652
38e3cc7
4c0a097
6eb73ed
114fec0
b9696da
5077401
3f5bab2
0ff8e00
cb35c6c
afe4c91
45f4eb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,89 +1,85 @@ | ||||||||||||||
| # Contributing to the Kubeflow SDK | ||||||||||||||
|
|
||||||||||||||
| Thank you for your interest in contributing to the Kubeflow SDK! | ||||||||||||||
| This guide explains how to contribute to the Kubeflow SDK project. | ||||||||||||||
| For the Kubeflow SDK documentation, please check [the official Kubeflow documentation](https://www.kubeflow.org/docs/components/). | ||||||||||||||
|
|
||||||||||||||
| ## Getting Started | ||||||||||||||
|
|
||||||||||||||
| ### Prerequisites | ||||||||||||||
| - Python 3.8–3.11 | ||||||||||||||
| - [pip](https://pip.pypa.io/en/stable/) | ||||||||||||||
| ## Requirements | ||||||||||||||
| - [Supported Python version](./pyproject.toml#L4) | ||||||||||||||
| - [pre-commit](https://pre-commit.com/) | ||||||||||||||
| - [uv](https://docs.astral.sh/uv/getting-started/installation/) | ||||||||||||||
|
|
||||||||||||||
| ### Setting Up Your Development Environment | ||||||||||||||
| Clone the repository: | ||||||||||||||
| ```sh | ||||||||||||||
| git clone https://github.com/kubeflow/sdk.git | ||||||||||||||
| cd sdk | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| Create a virtual environment and activate it: | ||||||||||||||
| ```sh | ||||||||||||||
| python3 -m venv .venv | ||||||||||||||
| source .venv/bin/activate | ||||||||||||||
| ``` | ||||||||||||||
| ## Development | ||||||||||||||
|
|
||||||||||||||
| Install dependencies in editable mode: | ||||||||||||||
| ```sh | ||||||||||||||
| pip install -e . | ||||||||||||||
| ``` | ||||||||||||||
| The Kubeflow SDK project includes a Makefile with several helpful commands to streamline your development workflow. | ||||||||||||||
|
|
||||||||||||||
| To install all dependencies (including dev tools) and create virtual environment, run | ||||||||||||||
|
|
||||||||||||||
| #### Development Build (Optional) | ||||||||||||||
| To install development tools and the latest API modules directly from the master branch: | ||||||||||||||
| ```sh | ||||||||||||||
| uv pip install -e . --group dev | ||||||||||||||
| make install-dev | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| ## Development Workflow | ||||||||||||||
|
|
||||||||||||||
| ### Pre-commit | ||||||||||||||
| We use pre-commit to ensure consistent code formatting. To enable pre-commit hooks, run: | ||||||||||||||
| ```sh | ||||||||||||||
| ```shell | ||||||||||||||
| pre-commit install | ||||||||||||||
| ``` | ||||||||||||||
| To run all hooks manually: | ||||||||||||||
| ```sh | ||||||||||||||
| pre-commit run --all-files | ||||||||||||||
|
||||||||||||||
| ```shell | |
| pre-commit install | |
| ``` | |
| To run all hooks manually: | |
| ```sh | |
| pre-commit run --all-files |
Outdated
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.
This duplicates https://github.com/szaher/sdk/blob/update-contrib/CONTRIBUTING.md#pre-commit
Could we have one clear section?
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.
I agree, maybe we can keep Coding Style as a subsection under Development, e.g.
## Development
...
### Coding Style
Info about pre-commit + `make verify` to check formatting.
## Testing
## Best Practices
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.
Yep, sounds great to me
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.
done
szaher marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -51,7 +51,7 @@ ruff: ## Install Ruff | |||||
| @uvx ruff --help &> /dev/null || uv tool install ruff | ||||||
|
|
||||||
| .PHONY: verify | ||||||
| verify: uv uv-venv ruff ## install all required tools | ||||||
| verify: install-dev ## install all required tools | ||||||
| @uv lock --check | ||||||
| @uvx ruff check --show-fixes | ||||||
|
|
||||||
|
|
@@ -75,3 +75,11 @@ ifeq ($(report),xml) | |||||
| else | ||||||
| @uv run coverage html | ||||||
| endif | ||||||
|
|
||||||
szaher marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| .PHONY: install-dev | ||||||
| install-dev: uv uv-venv ruff ## Install uv, create .venv, sync deps; DEV=1 to include dev group; EXTRAS=comma,list for extras | ||||||
| @echo "Using virtual environment at: $(VENV_DIR)" | ||||||
| @echo "Syncing dependencies with uv..." | ||||||
| @uv sync --group dev | ||||||
|
||||||
| @uv sync --group dev | |
| @uv sync |
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.
@kramaranya Is that true ?
IIUC, by default it will use the default installation (e.g. pip install kubeflow), however in make install we should install dev version.
So we should it to uv sync --group dev as @szaher proposed.
@szaher Maybe we should rename this script to make it more explicit:
make install-dev
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.
Yes, uv sync installs dev group by default https://docs.astral.sh/uv/concepts/projects/dependencies/#default-groups
We should remove --group dev from uv sync. The default installation (similar to pip install kubeflow) with no dev tools would be uv sync --no-dev.
@szaher could you update it accordingly?
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.
@kramaranya since uv sync and uv sync --dev has the same effect. dev is a default group in the uv dependency groups so both have the same effect.
keeping --group dev isn't harmful and is more explicit.
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.
I see, thanks for letting us know @kramaranya!
Actually, I might agree with @szaher to use uv sync --group dev since it makes more explicit for the developers that we use dev installation, but I am fine with any.
@astefanutti @Electronic-Waste Any thoughts ?
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.
I'm fine either way, but I think --group dev just makes the command a bit noisier
Uh oh!
There was an error while loading. Please reload this page.