chore: Update CONTRIBUTING.md to use uv #41

szaher · 2025-07-17T11:54:26Z

What this PR does / why we need it:

Update CONTRIBUTING.md to use uv
Add pre-commit as dev dependency
add pytest as dev dependency

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):

Fixes #

Checklist:

Docs included if any changes are user facing

Signed-off-by: Saad Zaher <szaher@redhat.com>

eoinfennessy

Looks good. I added some comments and suggestions.

CONTRIBUTING.md

Makefile

CONTRIBUTING.md

kramaranya · 2025-08-07T15:49:01Z

Thank you @szaher!
Could you rebase against main? There are some important changes that need to be included

Signed-off-by: Saad Zaher <szaher@redhat.com>

coveralls · 2025-08-22T00:58:36Z

Pull Request Test Coverage Report for Build 17142997184

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 64.877%

Totals
Change from base Build 17059769083:	0.3%
Covered Lines:	290
Relevant Lines:	447

💛 - Coveralls

coveralls · 2025-08-22T00:58:36Z

Pull Request Test Coverage Report for Build 17142997184

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 64.877%

Totals
Change from base Build 17059769083:	0.3%
Covered Lines:	290
Relevant Lines:	447

💛 - Coveralls

coveralls · 2025-08-22T00:58:37Z

Pull Request Test Coverage Report for Build 17582757769

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
31 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.9%) to 71.053%

Files with Coverage Reduction	New Missed Lines	%
kubeflow/trainer/utils/utils.py	31	63.94%

Totals
Change from base Build 17487937699:	0.9%
Covered Lines:	297
Relevant Lines:	418

💛 - Coveralls

szaher · 2025-08-22T00:59:58Z

@andreyvelich can we get this one reviewed?

andreyvelich

Thanks @szaher, I've added some thoughts.
/assign @astefanutti @Electronic-Waste @kramaranya

CONTRIBUTING.md

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>

Signed-off-by: Saad Zaher <eng.szaher@gmail.com>

…ntrib

Signed-off-by: Saad Zaher <szaher@redhat.com>

CONTRIBUTING.md

andreyvelich · 2025-09-07T22:16:50Z

CONTRIBUTING.md

+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):

-Install dependencies in editable mode:
 ```sh
-pip install -e .
+make install
 ```

-#### Development Build (Optional)
+#### API Models
+
 To install development tools and the latest API modules directly from the master branch:
-```sh
-uv pip install -e . --group dev
-```

-## Development Workflow
+```shell
+uv sync --dev
+```


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.

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

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>

CONTRIBUTING.md

andreyvelich · 2025-09-08T14:05:39Z

CONTRIBUTING.md

+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):


I don't think that we need separate section for installing uv and sync API modules, since it is included in the make install command.

Suggested change

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

@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.

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?

Makefile

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>

kramaranya

Thank you @szaher

kramaranya · 2025-09-08T15:10:20Z

CONTRIBUTING.md

+
 ```sh
-python3 -m venv .venv
-source .venv/bin/activate
-```
+make install

-Install dependencies in editable mode:
 ```sh
-pip install -e .
+make install
 ```



Please, clean this up so we just have make install

kramaranya · 2025-09-08T15:10:47Z

CONTRIBUTING.md

+```shell
+uv sync --dev
+```


We don't need this since we have make install

kramaranya · 2025-09-08T15:15:05Z

Makefile

+install: 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 already includes dev group

Suggested change

@uv sync --group dev

@uv sync

@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

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?

@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.

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 ?

I'm fine either way, but I think --group dev just makes the command a bit noisier

CONTRIBUTING.md

kramaranya · 2025-09-08T15:21:44Z

CONTRIBUTING.md

+To fix all lint issues automatically using ruff append `--fix` to your ruff check command.
+
+```shell
+uv run ruff check --fix .
 ```
-To lint:
-```sh
-flake8 --exclude .venv
+
+To auto-format, lint all files
+
+```shell
+uv run ruff format .


Feels like a lot of commands, shall we maybe add make fix or make format instead?

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.

Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich

Thanks @szaher!
/lgtm
/assign @kramaranya

kramaranya · 2025-09-09T09:12:24Z

CONTRIBUTING.md

+## Coding Style
+Make sure to install [pre-commit](https://pre-commit.com/) (`uv pip install pre-commit`) and run `pre-commit install` from the root of the repository at least once before creating git commits.
+
+The pre-commit hooks ensure code quality and consistency. They are executed in CI. PRs that fail to comply with the hooks will not be able to pass the corresponding CI gate. The hooks are only executed against staged files unless you run `pre-commit run --all`, in which case, they'll be executed against every file in the repository.
+
+Specific programmatically generated files listed in the `exclude` field in [.pre-commit-config.yaml](.pre-commit-config.yaml) are deliberately excluded from the hooks.
+


This duplicates https://github.com/szaher/sdk/blob/update-contrib/CONTRIBUTING.md#pre-commit
Could we have one clear section?

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

Yep, sounds great to me

Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich · 2025-09-09T12:00:39Z

CONTRIBUTING.md

+```shell
 pre-commit install
 ```
 To run all hooks manually:
 ```sh
 pre-commit run --all-files


You can remove those sections, since you explain it here: https://github.com/kubeflow/sdk/blob/afe4c9160412090c885e9ab5c214d34c6ea788c3/CONTRIBUTING.md#coding-style

Suggested change

```shell

pre-commit install

```

To run all hooks manually:

```sh

pre-commit run --all-files

andreyvelich · 2025-09-09T12:00:47Z

CONTRIBUTING.md

+#### Pre-commit
 We use pre-commit to ensure consistent code formatting. To enable pre-commit hooks, run:


Suggested change

#### Pre-commit

We use pre-commit to ensure consistent code formatting. To enable pre-commit hooks, run:

Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich

Thank you @szaher!
/lgtm
/assign @kramaranya

kramaranya · 2025-09-09T12:42:52Z

Awesome, thank you @szaher!
/lgtm

andreyvelich · 2025-09-09T13:07:28Z

/approve

google-oss-prow · 2025-09-09T13:07:38Z

[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:

~~OWNERS~~ [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@andreyvelich

* Update CONTRIBUTING.md to use uv Signed-off-by: Saad Zaher <szaher@redhat.com> * remove .flake8 & update .gitignore Signed-off-by: Saad Zaher <szaher@redhat.com> * Fix merge conflict Signed-off-by: Saad Zaher <szaher@redhat.com> * checkout Makefile, pyproject, lock file from main branch Signed-off-by: Saad Zaher <szaher@redhat.com> * update gitignore Signed-off-by: Saad Zaher <szaher@redhat.com> * Update CONTRIBUTING.md Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com> * Add make install & foramt Signed-off-by: Saad Zaher <eng.szaher@gmail.com> * Remove uv pip install dev dependencies as we are moving to `dependency-groups` instead of `optional-dependencies` because pypi.org doesn't support git dependencies we need ot use `uv sync` instead of `uv pip ...` which will automatically sync all default groups (dev is a default group in uv) Signed-off-by: Saad Zaher <szaher@redhat.com> * fix typo in the contributing.md file Signed-off-by: Saad Zaher <szaher@redhat.com> * Update Makefile Signed-off-by: Saad Zaher <szaher@redhat.com> * update structure Signed-off-by: Saad Zaher <szaher@redhat.com> * rephrase uv install Signed-off-by: Saad Zaher <szaher@redhat.com> * update install step at Makefile Signed-off-by: Saad Zaher <szaher@redhat.com> * Update CONTRIBUTING.md Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com> * Apply suggestion from @andreyvelich Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com> * Apply suggestion from @andreyvelich Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com> * update Make target verify to use install Signed-off-by: Saad Zaher <szaher@redhat.com> * fix formatting in contributing guide Signed-off-by: Saad Zaher <szaher@redhat.com> * remove redundant commands Signed-off-by: Saad Zaher <szaher@redhat.com> * use make install-dev instead of make install Signed-off-by: Saad Zaher <szaher@redhat.com> * update install target reference Signed-off-by: Saad Zaher <szaher@redhat.com> * remove group dev from make install-dev target Signed-off-by: Saad Zaher <szaher@redhat.com> * move coding style to dev Signed-off-by: Saad Zaher <szaher@redhat.com> * remove pre-commit stuff from contributing guide Signed-off-by: Saad Zaher <szaher@redhat.com> --------- Signed-off-by: Saad Zaher <szaher@redhat.com> Signed-off-by: Saad Zaher <eng.szaher@gmail.com> Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>

Update CONTRIBUTING.md to use uv

6eb90f4

Signed-off-by: Saad Zaher <szaher@redhat.com>

google-oss-prow bot requested review from Electronic-Waste, andreyvelich and astefanutti July 17, 2025 11:54

google-oss-prow bot added the size/L label Jul 17, 2025

remove .flake8 & update .gitignore

665f199

Signed-off-by: Saad Zaher <szaher@redhat.com>

eoinfennessy reviewed Jul 17, 2025

View reviewed changes

szaher added 2 commits August 22, 2025 01:52

Merge branch 'main' into update-contrib

c91307e

Fix merge conflict

bffa122

Signed-off-by: Saad Zaher <szaher@redhat.com>

google-oss-prow bot added size/XL and removed size/L labels Aug 22, 2025

checkout Makefile, pyproject, lock file from main branch

1c1c886

Signed-off-by: Saad Zaher <szaher@redhat.com>

google-oss-prow bot added size/M and removed size/XL labels Aug 22, 2025

update gitignore

1998f0b

Signed-off-by: Saad Zaher <szaher@redhat.com>

szaher changed the title ~~Update CONTRIBUTING.md to use uv~~ (chore ): Update CONTRIBUTING.md to use uv Aug 22, 2025

szaher changed the title ~~(chore ): Update CONTRIBUTING.md to use uv~~ (chore): Update CONTRIBUTING.md to use uv Aug 22, 2025

szaher changed the title ~~(chore): Update CONTRIBUTING.md to use uv~~ chore: Update CONTRIBUTING.md to use uv Aug 22, 2025

andreyvelich reviewed Aug 22, 2025

View reviewed changes

google-oss-prow bot assigned astefanutti, Electronic-Waste and kramaranya Aug 22, 2025

szaher and others added 3 commits August 23, 2025 00:31

Update CONTRIBUTING.md

760ae1a

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>

Add make install & foramt

8d88050

Signed-off-by: Saad Zaher <eng.szaher@gmail.com>

Merge branch 'update-contrib' of github.com:szaher/sdk into update-co…

6c4dd97

…ntrib

update install step at Makefile

958d652

Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich reviewed Sep 7, 2025

View reviewed changes

Update CONTRIBUTING.md

38e3cc7

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich reviewed Sep 8, 2025

View reviewed changes

szaher and others added 4 commits September 8, 2025 15:49

Apply suggestion from @andreyvelich

4c0a097

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>

Apply suggestion from @andreyvelich

6eb73ed

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Saad Zaher <szaher@redhat.com>

update Make target verify to use install

114fec0

Signed-off-by: Saad Zaher <szaher@redhat.com>

fix formatting in contributing guide

b9696da

Signed-off-by: Saad Zaher <szaher@redhat.com>

kramaranya reviewed Sep 8, 2025

View reviewed changes

szaher added 3 commits September 9, 2025 00:02

remove redundant commands

5077401

Signed-off-by: Saad Zaher <szaher@redhat.com>

use make install-dev instead of make install

3f5bab2

Signed-off-by: Saad Zaher <szaher@redhat.com>

update install target reference

0ff8e00

Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich reviewed Sep 8, 2025

View reviewed changes

google-oss-prow bot assigned andreyvelich Sep 8, 2025

google-oss-prow bot added the lgtm label Sep 8, 2025

kramaranya reviewed Sep 9, 2025

View reviewed changes

remove group dev from make install-dev target

cb35c6c

Signed-off-by: Saad Zaher <szaher@redhat.com>

google-oss-prow bot removed the lgtm label Sep 9, 2025

move coding style to dev

afe4c91

Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich reviewed Sep 9, 2025

View reviewed changes

remove pre-commit stuff from contributing guide

45f4eb0

Signed-off-by: Saad Zaher <szaher@redhat.com>

andreyvelich reviewed Sep 9, 2025

View reviewed changes

google-oss-prow bot added the lgtm label Sep 9, 2025

google-oss-prow bot added the approved label Sep 9, 2025

google-oss-prow bot merged commit 6709dcf into kubeflow:main Sep 9, 2025
10 checks passed

szaher deleted the update-contrib branch September 9, 2025 13:31

		#### Pre-commit
		We use pre-commit to ensure consistent code formatting. To enable pre-commit hooks, run:

chore: Update CONTRIBUTING.md to use uv #41

chore: Update CONTRIBUTING.md to use uv #41

Conversation

szaher commented Jul 17, 2025

Uh oh!

eoinfennessy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kramaranya commented Aug 7, 2025

Uh oh!

coveralls commented Aug 22, 2025

Pull Request Test Coverage Report for Build 17142997184

Details

💛 - Coveralls

Uh oh!

coveralls commented Aug 22, 2025

Pull Request Test Coverage Report for Build 17142997184

Details

💛 - Coveralls

Uh oh!

coveralls commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17582757769

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

szaher commented Aug 22, 2025

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kramaranya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvelich Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvelich Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 22, 2025 •

edited

Loading

andreyvelich Sep 8, 2025 •

edited

Loading

andreyvelich Sep 9, 2025 •

edited

Loading

kramaranya Sep 8, 2025 •

edited

Loading

andreyvelich Sep 9, 2025 •

edited

Loading