-
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 18 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,119 @@ | ||||||||||||||||||||||||||||||||
| # 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 | ||||||||||||||||||||||||||||||||
| - Python 3.9–3.11 | ||||||||||||||||||||||||||||||||
| - [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 | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| The Kubeflow SDK project includes a Makefile with several helpful commands to streamline your development workflow. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Install uv if not installed [Official Docs](https://docs.astral.sh/uv/getting-started/installation/) or using the following command: | ||||||||||||||||||||||||||||||||
| ```shell | ||||||||||||||||||||||||||||||||
| make uv | ||||||||||||||||||||||||||||||||
szaher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||
| ### Install SDK & Dependencies | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Create a virtual environment and install all dependencies (including dev tools): | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| The Kubeflow SDK project includes a Makefile with several helpful commands to streamline your development workflow. | |
| Install uv if not installed [Official Docs](https://docs.astral.sh/uv/getting-started/installation/) or using the following command: | |
| ```shell | |
| make uv | |
| ``` | |
| ### Install SDK & Dependencies | |
| Create a virtual environment and install all dependencies (including dev tools): | |
| 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 | |
| ```sh | |
| make install |
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.
@szaher Can you fix formatting please: https://github.com/kubeflow/sdk/blob/114fec00669263b6f6302519cd90c570ad8fe4ed/CONTRIBUTING.md#development ?
API Models section also can be removed, since uv sync --dev is part of make install script.
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.
well, uv sync or make install will install the default sdk packages with api models == specific version.
the dev group should install api models == master branch
do we need to remove it give that this guide is for people to contribute so they should use the latest api models?
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.
Can you delete all of these and just run make verify in the Development section ?
Similar to Trainer: https://github.com/kubeflow/trainer/blob/master/CONTRIBUTING.md#development
That should install all required tools and configure uv environment.
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.
actually this one is very specific to installing api modules from master branch but the actual installation of dependencies and app is a couple of lines up.
I added make install
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.
We don't need this since we have make install
szaher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
You can remove those sections, since you explain it here: https://github.com/kubeflow/sdk/blob/afe4c9160412090c885e9ab5c214d34c6ea788c3/CONTRIBUTING.md#coding-style
| ```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
szaher marked this conversation as resolved.
Show resolved
Hide resolved
szaher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
Feels like a lot of commands, shall we maybe add make fix or make format instead?
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 would really recommend we keep like that for now.
- We can add that later in the ruff pr
- we need to support fix, format specific files or files if provided by the user
uv run ruff check --fix /PATH/TO/FILE
It could be also a separate issue to support individual components, we can create a tracking issue for it.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||
| install: uv uv-venv ruff ## Install uv, create .venv, sync deps; DEV=1 to include dev group; EXTRAS=comma,list for extras | ||||||
szaher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| @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.