Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .flake8

This file was deleted.

4 changes: 4 additions & 0 deletions .github/workflows/check-pr-title.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ on:
- reopened
- synchronize

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

jobs:
check:
name: Check PR Title
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/github-stale.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ on:
schedule:
- cron: "0 */5 * * *"

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

jobs:
stale:
runs-on: ubuntu-latest
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/test-e2e.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ name: E2E Test
on:
pull_request

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

jobs:
e2e-test:
name: E2E Test
Expand Down
17 changes: 14 additions & 3 deletions .github/workflows/test-python.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,27 @@ on:
push:
branches: [main]

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
Comment on lines +8 to +10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, we discussed before that this part is not needed given that we don't have limitations on concurrent runs due to GitHub enterprise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A use case is re-pushing the branch for the PR while the workflow is running. Even with no limitations, is that useful to keep the existing run going?

Copy link
Member

@andreyvelich andreyvelich Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be useful to compare results, but I am fine to cancel them for now.
@kramaranya Please can you apply the change to all GitHub action tests in that case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, updated in 4853da4


jobs:
test:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
python-version: ['3.9', '3.11']

name: Test (Python ${{ matrix.python-version }})

steps:
- uses: actions/checkout@v4

- name: Set up Python
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: '3.9'
python-version: ${{ matrix.python-version }}

- name: Verify
run: make verify
Expand All @@ -26,4 +37,4 @@ jobs:
- name: Upload coverage to Coveralls
uses: coverallsapp/github-action@v2
with:
github-token: ${{ secrets.GITHUB_TOKEN }} # Pass the GITHUB_TOKEN
github-token: ${{ secrets.GITHUB_TOKEN }}
6 changes: 6 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ cd python
pip install -e .
```

#### Development Build (Optional)
To install the latest API modules directly from the master branch:
```sh
pip install -e ".[dev]"
```

Install development tools:
```sh
pip install pytest black isort flake8 coverage pre-commit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, we can remove it since we have everything in dev, right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, updated in cb1c4df

Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ uv-venv:
echo "uv virtual environment already exists in $(VENV_DIR)."; \
fi

# make test-unit will produce html coverage by default. Run with `make test-unit report=xml` to produce xml report.
# make test-python will produce html coverage by default. Run with `make test-python report=xml` to produce xml report.
.PHONY: test-python
test-python: uv-venv
@uv pip install "./python[test]"
@uv pip install --project $(PY_DIR) --group dev "$(PY_DIR)[test,dev]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can we move ruff and test packages to dev too ?

In that case we have all tools required for development in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, shall we also add pre-commit package to dev?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, updated in cb1c4df!

@uv run coverage run --source=kubeflow.trainer.api.trainer_client,kubeflow.trainer.utils.utils -m pytest ./python/kubeflow/trainer/api/trainer_client_test.py
@uv run coverage report -m kubeflow/trainer/api/trainer_client.py kubeflow/trainer/utils/utils.py
ifeq ($(report),xml)
Expand Down
2 changes: 1 addition & 1 deletion python/kubeflow/trainer/api/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# flake8: noqa
# ruff: noqa

# import apis into api package

5 changes: 4 additions & 1 deletion python/pyproject.toml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also migrate to the official package as part of this PR: https://pypi.org/project/kubeflow-trainer-api/

Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,17 @@ classifiers = [
dependencies = [
"kubernetes>=27.2.0",
"pydantic>=2.10.0",
"kubeflow_trainer_api@git+https://github.com/kubeflow/trainer.git@master#subdirectory=api/python_api"
"kubeflow-trainer-api>=2.0.0",
]
[project.optional-dependencies]
test = [
"pytest>=7.0",
"pytest-mock>=3.10",
"coverage>=7.0",
]
dev = [
"kubeflow_trainer_api@git+https://github.com/kubeflow/trainer.git@master#subdirectory=api/python_api",
]

[project.urls]
Homepage = "https://github.com/kubeflow/sdk"
Expand Down
8 changes: 6 additions & 2 deletions python/uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.