Conversation
johannaSommer
left a comment
johannaSommer
left a comment
There was a problem hiding this comment.
Cool changes, we love a good cleanup! 🫧 Just a few things to adjust and let's also rebase once PRs this depends on are merged, then we are GTG!
There was a problem hiding this comment.
I think we definitely have to keep this file - I understand your comment in the description, we do installation in other places, but we use this workflow to check specifically other python versions and other OS types. Some packages like e.g. gptqmodel are not compatible with all OS
There was a problem hiding this comment.
Oh, good to know that Johanna! Then I'll bring it back after we merge this PR
There was a problem hiding this comment.
I restored the installation.yaml file 🙏
There was a problem hiding this comment.
the badges at the top of the README depend on this naming, can we also update them in this PR?
GreatBahram
force-pushed
the
feature-simplify-ci-files
branch
3 times, most recently
from
8788466 to
293747b
Compare
GreatBahram
force-pushed
the
feature-simplify-ci-files
branch
from
293747b to
8b84911
Compare
johannaSommer
left a comment
johannaSommer
left a comment
There was a problem hiding this comment.
Just one comment then we are already there I think!
|
|
||
| - uses: ./.github/actions/setup-uv-project | ||
|
|
||
| - name: Install dependencies |
There was a problem hiding this comment.
One quick question here, is the pip install -e . necessary? In my understanding this is taken care of by the uv setup action
There was a problem hiding this comment.
I checked this, you're totally right we don't need this 🙏
There was a problem hiding this comment.
Okay if this is the case, then theoretically if we have python strategy matrix in other workflows, we can potentially remove this one 🤔
There was a problem hiding this comment.
ah good point, to clarify, you're suggesting this because uv anyway resolves the packages for all specified python version from the pyproject.toml?
There was a problem hiding this comment.
yeah, exactly, since we have the setup-uv-project almost in all workflows
There was a problem hiding this comment.
💯 thats a great idea, then we could basically keep the installation tests but only with a matrix over macos/windows, agree? i love it, let's tackle it in a separate PR?
GreatBahram
force-pushed
the
feature-simplify-ci-files
branch
from
93bf1f2 to
49593c3
Compare
| - name: Set lint check output | ||
| id: lint-check | ||
| run: echo "success=true" >> $GITHUB_OUTPUT | ||
| test: |
There was a problem hiding this comment.
Note
Here we have the cpu_tests.yaml file content. It still run on the configured runner and in parallel to the linting job.
2 participants
Description
If agree with these changes, we can merge this after this PR, here my goal was to simplify the workflow files which means fewer workflows to maintain, changes include:
package_build.yaml→build.yamldocumentation.yaml→docs.yamllinting.yaml&tests.yamlinto one →tests.yaml(now includes both linting + test jobs they should run on their runner and in parallel)Related Issue
Fixes #(issue number)
Type of Change
How Has This Been Tested?
Checklist
Additional Notes