Skip to content

fix: wait for medium e2e to finish and pass #606

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix: wait for medium e2e to finish and pass #606

wants to merge 1 commit into from

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Jun 10, 2025 •
edited
Loading

the medium e2e should block a PR from merging. wait for the test to pass.

@mergify mergify bot added CI/CD Affects CI/CD configuration ci-failure labels Jun 10, 2025
@mergify mergify bot removed the ci-failure label Jun 10, 2025
booxter
booxter reviewed Jun 10, 2025
View reviewed changes
@cdoern cdoern force-pushed the wait-for-e2e branch 2 times, most recently from b33211d to 0937de3 Compare June 11, 2025 18:56
@mergify mergify bot added the ci-failure label Jun 11, 2025
@cdoern cdoern force-pushed the wait-for-e2e branch 3 times, most recently from 00f2c8a to cd4ae84 Compare June 11, 2025 19:01
@mergify mergify bot removed the ci-failure label Jun 11, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 11, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. @cdoern please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Jun 11, 2025
@mergify mergify bot removed the needs-rebase label Jun 11, 2025
@fynnsu
Copy link
Collaborator

fynnsu commented Jun 12, 2025

Unless I'm misunderstanding something, it seems like the status-check.yml job gets started immediately when e2e-nvidia-l40s-x4-sdk.yml runs, and just spins while repeatedly checking if the two tests are done.

However, I notice the status-check.yml has a default timeout of 1h. Presumably after this hour the status of the job is marked as a failure? Is e2e-nvidia-l40s-x4-sdk.yml guaranteed to finish within an hour? (the non-sdk e2e takes 4 hours currently). Do we need to increase this timeout?

Or could we do something like this, so that the status check is only launched after the two jobs finish?

  job3:
    if: ${{ always() }}
    needs: [job1, job2]

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/using-jobs-in-a-workflow#example-not-requiring-successful-dependent-jobs

@cdoern cdoern force-pushed the wait-for-e2e branch 6 times, most recently from f73b43d to f2391db Compare June 12, 2025 20:37
booxter
booxter reviewed Jun 20, 2025
View reviewed changes
Copy link
Contributor

@booxter booxter left a comment

Choose a reason for hiding this comment

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

It would be nice to check it works. But we cannot trigger the affected jobs since the PR branch is in fork repo, not in the main repo. Consider pushing it to main repo as a private branch and triggering the jobs against this test branch.

.github/mergify.yml
- files=requirements.txt
- files=requirements-dev.txt
- files=constraints-dev.txt
- files~=^\.github/workflows/.*\.yml$ # This workflow
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to trigger medium for a change in ANY workflow file, do we?

.github/mergify.yml
- files=constraints-dev.txt
- files~=^\.github/workflows/.*\.yml$ # This workflow
- and:
- files~=\.py$
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't these entries have - prefixes, since this section is to detect the situation when no listed template matches? (in which case we do NOT require check-success~=e2e-medium-workflow-complete)

.github/workflows/e2e-nvidia-l40s-x4-sdk.yml
description: 'pull request number or branch name'
required: true
default: 'main'
- ".github/workflows/*.yml" # This workflow
Copy link
Contributor

Choose a reason for hiding this comment

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

this change seems not helping - it triggers the job for unrelated workflow file changes.

.github/workflows/status-checks.yml
status-checks:
runs-on: ubuntu-latest
steps:
- name: "Harden Runner"
Copy link
Contributor

Choose a reason for hiding this comment

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

we are going to remove this step from all workflows; I'd rather drop it from the PR now instead of cleaning up later.

@mergify mergify bot added the ci-failure label Jun 22, 2025
@cdoern cdoern force-pushed the wait-for-e2e branch 2 times, most recently from 11f32f3 to 370b5e6 Compare June 25, 2025 13:25
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 25, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. @cdoern please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added needs-rebase and removed ci-failure labels Jun 25, 2025
@mergify mergify bot removed the needs-rebase label Jun 25, 2025
@mergify mergify bot mentioned this pull request Jun 25, 2025
Open
@cdoern
Copy link
Contributor Author

cdoern commented Jun 25, 2025

closing in favor of #627

@cdoern cdoern closed this Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@booxter booxter booxter left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CI/CD Affects CI/CD configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cdoern @fynnsu @booxter