-
Notifications
You must be signed in to change notification settings - Fork 10
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
CI: fewer triggers #457
CI: fewer triggers #457
Conversation
.github/workflows/build.yml
Outdated
name: Build for (${{ matrix.python-version }}, ${{ matrix.os }}) | ||
runs-on: ${{ matrix.os }} | ||
|
||
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: ['ubuntu-latest'] | ||
python-version: ['3.9', '3.10'] | ||
python-version: ['3.9'] |
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.
If we say that the package work with python 3.9 and 3.10, this needs to be tested and verified with the CI. Either we decide that we don't support python 3.9 anymore (because in case I'd go for the newer python and not for the older version), or we keep both versions in the CI.
I would keep both, I don't think it's critical in our case (testing on the two latest releases of python is pretty common and we've already removed python 3.8 at the beginning of the project). Also it could be that some unix based OS still need python 3.9 for let pytorch work. At the same time we want to guarantee that the package works with the latest release of python (for example because it's faster than 3.9)
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.
Newest python is actually 3.11 :). But let's stick to 3.9 and 3.10 for now, I think.
We could do 3.9 for the normal build and 3.10 for the coveralls build. There is currently 1 command present in the normal build that isn't in coveralls, I can add that one to the coveralls to basically make it an extension of normal build.
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.
Oh and it was released in October, so many months ago. Then in the early future we should think about adding 3.11 as well, and at that point we can evaluate how critical is to remove the 3.9 version.
For the building, I get that coveralls does the build itself so you want to test 3.9 with the build file and 3.10 with the coveralls file, because both builds are tested this way. But this is not the cleaner way of doing it, because these yml files are thought to have well separated functions and they originate the badges that we show in the readme:
The build should be verified separately from coveralls, which should be run only if the build jobs go well. There are ways or creating interdependent yml files and avoiding running stuff multiple times, but I didn't want to spend much time on implementing more elegant solutions and I sticked to the basic one. If you're interested of course go for it, and we can have a chat for looking together where to find such info, but otherwise I'd prefer to use the build file to actually verify the builds that we want to guarantee (3.9 and 3.10 so far), and then use one of the two versions for running coveralls.
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.
Very nice improvements :) and I agree that path-ignore is safer here. I only have a comment about removing the two builds from the build file, let me know what you want to do about it. For the rest, consider the PR approved!
Changes to workflows to reduce environmental impact of CI (see here):
closes #453
redo of #454