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

add slurm support for err_exit #69

Merged
merged 6 commits into from
Dec 11, 2023
Merged

add slurm support for err_exit #69

merged 6 commits into from
Dec 11, 2023

Conversation

guoqing-noaa
Copy link
Contributor

Resolve issue #68

This was referenced Nov 15, 2023
Merged
Merged
@guoqing-noaa
Copy link
Contributor Author

Thanks for your view, @edwardhartnett!

What should I do to get this PR move forward?

@guoqing-noaa guoqing-noaa mentioned this pull request Dec 8, 2023
Closed
@MatthewPyle-NOAA
Copy link

@Hang-Lei-NOAA I got a question about moving this PR forward. Is anything else needed for it? Thanks!

@Hang-Lei-NOAA
Copy link
Contributor

Hang-Lei-NOAA commented Dec 8, 2023 via email

@guoqing-noaa
Copy link
Contributor Author

@Hang-Lei-NOAA I think the C test error is not due to the change in this PR.
The error information is as follows:
CMake Error at /usr/local/share/cmake-3.27/Modules/CMakeDetermineFortranCompiler.cmake:33 (message):
CMake Error: CMAKE_Fortran_COMPILER not set, after EnableLanguage

This failed test is for Intel. Do we have an Intel compiler available in Github?

Upon further checking, it looks like the process of downloading and installing an Intel compiler failed. If someone can rerun the tests, it may pass?

aerorahul
aerorahul requested changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

a suggestion to work this logic in the context of SENDECF

ush/err_exit Show resolved Hide resolved
@guoqing-noaa @aerorahul
Update ush/err_exit based on Rahul's suggestion
8754e24 
Co-authored-by: Rahul Mahajan <aerorahul@users.noreply.github.com>
@guoqing-noaa
Copy link
Contributor Author

a suggestion to work this logic in the context of SENDECF

Thanks, Rahul! Just committed the change as suggested.

@guoqing-noaa
Copy link
Contributor Author

There might be something wrong with the C Intel test setting which is out the scope of this PR. Could someone check and fix it? Thanks!

@Hang-Lei-NOAA
Copy link
Contributor

Hang-Lei-NOAA commented Dec 11, 2023 via email

@guoqing-noaa
Copy link
Contributor Author

Please check to see the intel workflow: under .github/workflows/Intel.yml If the C test version have any comflicited parts with your commit.

Hi @Hang-Lei-NOAA Thanks for the information. I only made a change in one BASH file which does not involve any C/C++ codes and I did not touch any other codes. From my understanding, the current code base before my PR cannot run this C Intel test.

@aerorahul
Copy link
Contributor

a suggestion to work this logic in the context of SENDECF

Thanks, Rahul! Just committed the change as suggested.

@guoqing-noaa
I think the last 3 lines 70-72 needs to be removed from this script. They no longer are necessary and will result in an error.

@guoqing-noaa
Copy link
Contributor Author

a suggestion to work this logic in the context of SENDECF

Thanks, Rahul! Just committed the change as suggested.

@guoqing-noaa I think the last 3 lines 70-72 needs to be removed from this script. They no longer are necessary and will result in an error.

I am so sorry that I had used the github "commit suggested changes" button and did not check the final result. Thanks a lot for help. Now it is fixed.

@aerorahul aerorahul self-requested a review December 11, 2023 17:47
aerorahul
aerorahul approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

looks good.

@aerorahul
Copy link
Contributor

@edwardhartnett @Hang-Lei-NOAA
The failures in the CI are not a result of changes in this PR.
Can we please address these one way or another so the changes can be merged?

@aerorahul
Copy link
Contributor

@guoqing-noaa
Please replace this with the following and push again.

          sudo apt-get install intel-oneapi-dev-utilities intel-oneapi-mpi-devel intel-oneapi-openmp intel-oneapi-compiler-fortran-2023.2.1 intel-oneapi-compiler-dpcpp-cpp-and-cpp-classi

However, I think that will break the icx and ifx modes.
@AlexanderRichert-NOAA Can we get some assistance in getting this PR through?

@Hang-Lei-NOAA
Copy link
Contributor

Hang-Lei-NOAA commented Dec 11, 2023 via email

@AlexanderRichert-NOAA
Copy link
Contributor

Indeed, add '-2023.2.1' to the intel fortran and c compiler packages in the apt install line. This won't/shouldn't break the icx/ifx workflow, it just won't be using the absolute most recent versions.

@guoqing-noaa
Copy link
Contributor Author

Thanks all. I will try to make the CI test change suggested by Rahul.

@aerorahul
Copy link
Contributor

aerorahul commented Dec 11, 2023 •
edited
Loading

@guoqing-noaa
I made a typo in that line. I accidentally left out c-2023.2.1 at the end.

@aerorahul aerorahul merged commit d1db177 into NOAA-EMC:develop Dec 11, 2023
14 checks passed
@guoqing-noaa
Copy link
Contributor Author

Thanks a lot for your help! @aerorahul @MatthewPyle-NOAA @Hang-Lei-NOAA @AlexanderRichert-NOAA

@guoqing-noaa
Copy link
Contributor Author

@aerorahul @Hang-Lei-NOAA Can you make a new tag? I can provide this to @climbfuji
Thanks!

@aerorahul
Copy link
Contributor

@edwardhartnett has a draft for v2.2 started. Perhaps he can finish that and include this in that release.

@MatthewPyle-NOAA MatthewPyle-NOAA mentioned this pull request Dec 11, 2023
Closed
@guoqing-noaa
Copy link
Contributor Author

@edwardhartnett Will we have a new tag in the next 1 or 2 days?
If more time is needed for v2.2.0, can we go ahead to make a new tag like v2.1.1 for the current develop HEAD?
Thanks!

@climbfuji
Copy link
Collaborator

Thanks we have the new tag 2.1.1 in spack-stack since yesterday

@edwardhartnett
Copy link
Contributor

Sorry I've been slow to respond, I've been at the AGU.

Do you want a new release of prod_util?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aerorahul aerorahul aerorahul approved these changes

@edwardhartnett edwardhartnett edwardhartnett approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@guoqing-noaa @MatthewPyle-NOAA @Hang-Lei-NOAA @aerorahul @AlexanderRichert-NOAA @climbfuji @edwardhartnett