-
Notifications
You must be signed in to change notification settings - Fork 461
[WIP - testing integration] Switch lib-lint-and-test workflow to uv #4932
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR migrates the CI/CD workflow from tox to uv for library linting and testing, aiming to streamline dependency management and improve build performance.
Key Changes:
- Replaced tox-based testing with uv for dependency management and test execution
- Updated Python version support to require Python 3.10+ (from 3.8+)
- Removed setuptools version constraint and reorganized dependency specifications
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| library/pyproject.toml | Updated dependency constraints, added uv configuration with extra conflicts, changed minimum Python target to 3.10 |
| .github/workflows/lib-lint-and-test.yaml | Replaced tox with uv for all CI jobs, simplified test execution, added Codecov action for coverage uploads |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Docker Image Sizes
|
📊 Test coverage report
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| branches: | ||
| - develop | ||
| - releases/** | ||
| - release/** |
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.
In other workflows we use releases/**, although in branch names we use release (e.g. https://github.com/open-edge-platform/training_extensions/tree/release/2.6)
Leonardo, could you please clarify what we plan to use? @leoll2
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 have intention to change this to "release/**" for other workflows as well
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.
Good catch. I'd opt for release for compatibility with the existing release branches. The other workflows have to be updated accordingly.
| curl -Os https://uploader.codecov.io/latest/linux/codecov | ||
| chmod +x codecov | ||
| ./codecov -t ${{ secrets.CODECOV_TOKEN }} --sha $COMMIT_ID -U $HTTP_PROXY -f .tox/coverage_unit-test-${{ matrix.tox-env }}.xml -F ${{ matrix.tox-env }} | ||
| uses: codecov/codecov-action@5a1091511ad55cbe89839c7260b706298ca349f7 # v5.5.1 |
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 step will fail for PR from fork (by design, GH token for pull_request trigger has read-only permissions).
I would propose to add condition to skip this step for PR from forks (something like https://github.com/open-edge-platform/training_extensions/blob/develop/.github/workflows/ui-lint-and-test.yaml#L259)
library/pyproject.toml
Outdated
| description = "OpenVINO™ Training Extensions: Train, Evaluate, Optimize, Deploy Computer Vision Models via OpenVINO™" | ||
| readme = "README.md" | ||
| requires-python = ">=3.10" | ||
| requires-python = ">=3.10,<3.13" |
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.
In app we use 3.13 https://github.com/open-edge-platform/training_extensions/blob/develop/application/docker/Dockerfile#L6
Will it somehow affect lib usage in application?
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.
OTX supports Python 3.13, so <3.13 is wrong here. I would leave this line unchanged: ">=3.10".
library/pyproject.toml
Outdated
| "typeguard>=4.3,<4.5", | ||
| # TODO(ashwinvaidya17): https://github.com/openvinotoolkit/anomalib/issues/2126 | ||
| "setuptools<70", | ||
| "setuptools", |
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 propose to firstly merge this PR #4903, as there are some chages in toml as well, e.g.
"setuptools==78.1.1"
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, indeed. Also this would help to integration test with adaptive batch size to succeed I reckon.
| run: | | ||
| pip install '.[dev]' | ||
| - name: Code quality checks | ||
| run: uv sync --locked --extra dev --extra cuda --no-extra xpu |
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.
Is the extra cuda necessary for the tests to pass? Does it even work on the standard GPU-less Github runners?
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.
Nice catch, indeed the cuda is not required to pass the quality check. Wanted to keep it the same as next tests. Though would revert back. Thanks!
| - name: Run pre-commit checks | ||
| working-directory: library | ||
| run: tox r -vv -e pre-commit | ||
| run: uv run --frozen --extra dev --extra cuda --no-extra xpu pre-commit run --all-files |
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 already install the dependencies in the previous step, I think --frozen --extra dev --extra cuda --no-extra xpu is redundant.
|
/run instance_segmentation |
| pip install '.[dev]' | ||
| - name: Run Integration Test | ||
| uv lock --check | ||
| uv sync --frozen --extra cuda --no-extra xpu |
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.
FYI I have just merged a CPU only mode (--extra cpu, #4972). I don't know if the integration tests require a GPU to pass, probably yes; if not, you may consider switching to this CPU installation.
| matrix: | ||
| include: | ||
| - task: "multi_class_cls" | ||
| python-version: "3.12" |
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 are setting python-version to 3.12 for all of matrix builds wouldn't it be better to set single env for whole job?
Switching lib-lint-and-test.yaml workflow to uv from tox
🚀 Integration Run Summary