Skip to content
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

Sanity check CI #939

Closed
wants to merge 8 commits into from
Closed

Sanity check CI #939

wants to merge 8 commits into from

Conversation

majosm
Copy link
Collaborator

@majosm majosm commented Jul 27, 2023

Seeing some strange things in #924...

@majosm majosm added the nomerge Not meant for merging label Jul 27, 2023
@majosm
Copy link
Collaborator Author

majosm commented Jul 27, 2023

@illinois-ceesd/developers Anyone have any guesses as to how the CI here could be picking up tests from main that don't yet exist in this branch? I saw this in #924 and was perplexed. CI finds 897 tests; when I run pytest on this branch locally I see 892. It looks like one of them is a doc test that I'm not running locally, but I noticed in CI it's also running the radiation tests:

2.92s call     test/test_multiphysics.py::test_thermally_coupled_fluid_wall_with_radiation[<PyOpenCLArrayContext for <pyopencl.Device 'cpu-Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz' on 'Portable Computing Language'>>-True-3]

...which don't yet exist in this branch. 🤯 Anyone have any idea what's going on here? The fix is easy enough (just update the branch), but I'm pretty confused by this behavior.

@matthiasdiener
Copy link
Member

And for some reason, now the CI doesn't run at all?! 🤯

@majosm majosm closed this Jul 28, 2023
@majosm majosm reopened this Jul 28, 2023
@majosm
Copy link
Collaborator Author

majosm commented Jul 28, 2023

@matthiasdiener I moved the debug insertions around a little to avoid the conflict. Looks like CI is running now.

@matthiasdiener
Copy link
Member

From the most recent log:

2023-07-28T15:15:09.9868687Z ##[group]Determining the checkout info
2023-07-28T15:15:09.9869286Z ##[endgroup]
2023-07-28T15:15:09.9869809Z ##[group]Checking out the ref
2023-07-28T15:15:09.9870740Z [command]/usr/bin/git checkout --progress --force refs/remotes/pull/939/merge
2023-07-28T15:15:10.0110270Z Note: switching to 'refs/remotes/pull/939/merge'.
2023-07-28T15:15:10.0110597Z 
2023-07-28T15:15:10.0111022Z You are in 'detached HEAD' state. You can look around, make experimental
2023-07-28T15:15:10.0111665Z changes and commit them, and you can discard any commits you make in this
2023-07-28T15:15:10.0112309Z state without impacting any branches by switching back to a branch.
2023-07-28T15:15:10.0112653Z 
2023-07-28T15:15:10.0112922Z If you want to create a new branch to retain commits you create, you may
2023-07-28T15:15:10.0113636Z do so (now or later) by using -c with the switch command. Example:
2023-07-28T15:15:10.0113961Z 
2023-07-28T15:15:10.0114219Z   git switch -c <new-branch-name>
2023-07-28T15:15:10.0114483Z 
2023-07-28T15:15:10.0114649Z Or undo this operation with:
2023-07-28T15:15:10.0114895Z 
2023-07-28T15:15:10.0115011Z   git switch -
2023-07-28T15:15:10.0115218Z 
2023-07-28T15:15:10.0115535Z Turn off this advice by setting config variable advice.detachedHead to false
2023-07-28T15:15:10.0115913Z 
2023-07-28T15:15:10.0116300Z HEAD is now at 826792e Merge 0f3dee328c011b774bda968d97d4f8b30cfc81c1 into 8e0468181cd67ea4a5059840f33d85ebd2aeea53
2023-07-28T15:15:10.0125377Z ##[endgroup]

So this merges 0f3dee3 (ie, current state of ci-sanity-check) into 8e04681 (ie, current state of main).

@majosm
Copy link
Collaborator Author

majosm commented Jul 31, 2023

So this merges 0f3dee3 (ie, current state of ci-sanity-check) into 8e04681 (ie, current state of main).

Do we know if that's Github doing that or the install script?

@matthiasdiener
Copy link
Member

matthiasdiener commented Jul 31, 2023

So this merges 0f3dee3 (ie, current state of ci-sanity-check) into 8e04681 (ie, current state of main).

Do we know if that's Github doing that or the install script?

This is done by the actions/checkout step (ie, not our install script), e.g. here. Is this the behavior we expect? This PR is targeting the main branch of mirgecom, so the CI tests merge this branch into the current main, which would be the same behavior if this PR was actually merged, I think.

@majosm
Copy link
Collaborator Author

majosm commented Jul 31, 2023

So this merges 0f3dee3 (ie, current state of ci-sanity-check) into 8e04681 (ie, current state of main).

Do we know if that's Github doing that or the install script?

This is done by the actions/checkout step (ie, not our install script), e.g. here. Is this the behavior we expect? This PR is targeting the main branch of mirgecom, so the CI tests merge this branch into the current main, which would be the same behavior if this PR was actually merged, I think.

If Github does it, then I guess it's fine. 🤷‍♂️ Just surprising to see different tests being run when running locally vs. here. And Github saying it's out of date but then it's magically up to date when CI runs is kind of jarring. 🙂 Oh well.

@majosm majosm closed this Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nomerge Not meant for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants