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.

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

CI: workaround for push issue #57

Closed
wants to merge 3 commits into from
Closed

CI: workaround for push issue #57

wants to merge 3 commits into from

Conversation

junghans
Copy link
Collaborator

@junghans junghans commented Feb 3, 2025 •
edited
Loading

Hacky workaround for #56 by adding a space after the 1st build to force a rebuild.

@junghans junghans self-assigned this Feb 3, 2025
@masterleinad masterleinad mentioned this pull request Feb 3, 2025
Closed
@junghans junghans marked this pull request as ready for review February 3, 2025 22:13
@junghans junghans enabled auto-merge February 3, 2025 22:22
@junghans
Copy link
Collaborator Author

junghans commented Feb 4, 2025

@masterleinad can we merge this workaround?

@dalg24 dalg24 disabled auto-merge February 4, 2025 16:46
dalg24
dalg24 requested changes Feb 4, 2025
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Please provide a proper description

.github/workflows/continuous-integration-workflow.yml Outdated
Comment on lines 29 to 30
- name: Checkout CI repo
uses: actions/checkout@v4
Copy link
Member

Choose a reason for hiding this comment

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

Why did we add this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now we need to checkout the Dockerfiles as we modify them below. Previously the docker action did that for us automatically.

.github/workflows/continuous-integration-workflow.yml Outdated
@@ -75,11 +77,11 @@ jobs:
-DKokkos_ENABLE_OPENMP=ON && \
cmake --build builddir --parallel 2 && \
ctest --test-dir builddir --output-on-failure"
sed -i '26s/$/ /' ${{ matrix.config.dockerfile }} # workaround for kokkos/ci-containers#56
Copy link
Member

Choose a reason for hiding this comment

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

What is that doing and why is it needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a workaround we are adding a space to the end of the line to force docker to "rebuild" line 26 and later commands.

@junghans junghans requested a review from dalg24 February 4, 2025 21:25
@junghans
Copy link
Collaborator Author

junghans commented Feb 4, 2025

Please provide a proper description

Sorry, I updated it above.

I wish I could find a better hack.

@junghans
Copy link
Collaborator Author

junghans commented Feb 4, 2025

Now

98% tests passed, 1 tests failed out of 55

Label Time Summary:
Kokkos    = 185.16 sec*proc (49 tests)

Total Test time (real) = 355.63 sec

The following tests FAILED:
	 35 - Kokkos_UnitTest_Random (Failed)
Errors while running CTest

not sure that has much to do with this PR.

dalg24
dalg24 reviewed Feb 4, 2025
View reviewed changes
.github/workflows/continuous-integration-workflow.yml Outdated
@@ -75,11 +77,11 @@ jobs:
-DKokkos_ENABLE_OPENMP=ON && \
cmake --build builddir --parallel 2 && \
ctest --test-dir builddir --output-on-failure"
sed -i '26s/$/ /' ${{ matrix.config.dockerfile }} # force rebuild by adding a space (kokkos/ci-containers#56)
Copy link
Member

Choose a reason for hiding this comment

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

Why line 26?

ci-containers/ubuntu

Line 26 in 79beb83

ENV LD_LIBRARY_PATH=${INTEL:+/opt/intel/oneapi/compiler/latest/linux/compiler/lib/intel64:/opt/intel/oneapi/mkl/latest/lib/intel64}${LD_LIBRARY_PATH:+:}${LD_LIBRARY_PATH}

ci-containers/fedora

Line 26 in 79beb83

USER kokkos

The opensuse file only has 13 lines.

Does that mean prepend a space to the 26th line?
If so please update the comment so that I don't wonder again what this is actually doing next time I look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was because it fails on line 26 for ubuntu.

@dalg24
Copy link
Member

dalg24 commented Feb 4, 2025

Hacky workaround for #56 by adding a space after the 1st build to force a rebuild.

Is there a particular reason why we cannot use the no-cache option.
I found out about it here https://github.com/docker/build-push-action/blob/v6.13.0/README.md#customizing

@junghans
Copy link
Collaborator Author

junghans commented Feb 5, 2025

Hacky workaround for #56 by adding a space after the 1st build to force a rebuild.

Is there a particular reason why we cannot use the no-cache option. I found out about it here https://github.com/docker/build-push-action/blob/v6.13.0/README.md#customizing

Let me try that....

@junghans junghans requested a review from dalg24 February 5, 2025 01:08
junghans
junghans commented Feb 5, 2025
View reviewed changes
.github/workflows/continuous-integration-workflow.yml
@@ -77,9 +77,10 @@ jobs:
ctest --test-dir builddir --output-on-failure"
- name: Push the image into GitHub Container Registry
uses: docker/build-push-action@v6
if: ${{ github.repository_owner == 'kokkos' && ( github.event_name == 'push' || github.event_name == 'schedule' ) }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could discuss to add this back, the step is relatively cheap except for the intel container and I prefer to run the whole CI as part of any PR.

@junghans
Copy link
Collaborator Author

junghans commented Feb 5, 2025

https://github.com/kokkos/ci-containers/actions/runs/13147752154
The issue seems to have solved itself.

@junghans junghans closed this Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masterleinad masterleinad Awaiting requested review from masterleinad

@dalg24 dalg24 Awaiting requested review from dalg24

Assignees

@junghans junghans

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@junghans @dalg24