-
Notifications
You must be signed in to change notification settings - Fork 442
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
[Test] Reduce Katib GitHub Action Runs #2036
[Test] Reduce Katib GitHub Action Runs #2036
Conversation
|
@DomFleischmann @knkski @ca-scribner
|
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.
Looks much clear ! Thanks for your efforts.
Just one question about Remove push execution from all unit/lint tests.
If there is no master branch protection rule such as Require branches to be up to date before merging
, I think we need to check such tests after the PR is merged too. WDYT ?
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.
Great!
This error is seen in other PRs as well. Can you help fix it at the earliest? @DomFleischmann @knkski @ca-scribner |
I am not sure if we need to publish all images during every commit. We will have to reconsider as this will slow down the whole development process. The user can build images any time using https://github.com/kubeflow/katib/blob/master/scripts/v1beta1/build.sh |
concurrency: | ||
group: ${{ github.workflow }}-${{ github.ref }} | ||
cancel-in-progress: true | ||
|
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.
@andreyvelich I'm not fully in sync with why we need to use concurrency
. Could you provide some more context?
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.
It is useful when the developer push multiple commits on an open PR in short period.
#2024 (comment)
To cancel previous-running workflows and run with new source codes.
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.
Yes, @anencore94 is right, when the newer commit has been pushed, previous workflows are canceled.
@kimwnasptd You can check an example here: https://github.com/kubeflow/katib/actions/runs/3534409566
This seems to be because the we are using |
@johnugeorge I don't think it will slow down the UI development. I disabled the most time consuming building suggestion images on pull requests: katib/.github/workflows/publish-algorithm-images.yaml Lines 5 to 7 in 01f4bcd
Do you have any concerns to have these actions when PR is merged ? I think, we have users who want to get the latest Katib features, and they can easily bump all of their images to the particular commit. If we don't push images of every commit, users have to check what commit they should take for each image. |
The Charmed seems to be working now: https://github.com/kubeflow/katib/actions/runs/3564777436/jobs/5989685476 |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, anencore94, johnugeorge, terrytangyuan 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:
Approvers can indicate their approval by writing |
@andreyvelich Thanks for this improvement! |
Related: #2024.
I've made few changes to improve our Actions execution.
postgres
e2e from all test and created new e2e test:e2e-test-ui-random-search-postgres.yaml
. In that test we check that Katib UI can be deployed on minikube + run random search withpostgres
DB.shellcheck
and YAML lint to the 1 workflow. I think, in the future PRs we should combine all unit test/lint checks in the 1 workflow.push
execution from all unit/lint tests.cancel-in-progress
flag to our e2e workflows./assign @johnugeorge @tenzen-y @anencore94 @kimwnasptd @orfeas-k