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

Update 'devbuild.sh', module files, and directory names #9

Merged
merged 30 commits into from
Jul 7, 2023

Conversation

chan-hoo
Copy link
Contributor

@chan-hoo chan-hoo commented Jul 3, 2023

DESCRIPTION OF CHANGES:

  • Updates devbuild.sh and adds devclean.sh
  • Changes the directory names: bin -> exec, src -> sorc
  • Moves the module files to modulefiles and updates modules with the latest ones.
  • Updates the hash of UFS_UTILS.

TESTS CONDUCTED:

  • Machines/platforms: wcoss2 (cactus), hera, orion

ISSUE:

@chan-hoo
Copy link
Contributor Author

chan-hoo commented Jul 3, 2023

@MatthewPyle-NOAA, could you give the code owners (listed in .github/CODEOWNERS) the write access to this repository? Then, they will be set as the default PR reviewers when a new PR is submitted. I can see the error message This CODEOWNERS file contains errors. This is because they don't have the write access. If you think the file (.github/CONDEOWNERS) needs to be modified, please let me know.

```
./devbuild.sh -p=[machine]
```
where `[machine]` is `wcoss2`, `hera`, `orion`, or `jet`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As can be seen in this new README file, this PR makes the build steps work up to Step 3 on wcoss2, hera and orion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chan-hoo Thank you so much for working on this so quickly. Since dev-sci will be used by both GSL and EMC, I add @hu5970 as a reviewer so that GSL can get an understanding of how the modification will affect the future update of RRFS-B on JET.

Copy link
Contributor Author

@chan-hoo chan-hoo Jul 3, 2023

Choose a reason for hiding this comment

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

Thanks. As described above, the rrfs-workflow does not work yet (even with this PR). This PR only makes the 'build' step work well. I've tested it on wcoss2, hera, and orion. I don't have access to Jet. Are there any other machines to be tested?

Copy link
Contributor

Choose a reason for hiding this comment

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

@chan-hoo I think the RRFS workflow for operations will only need to be supported on WCOSS2, Hera, Jet, and Orion. So at some point we will want to remove the logic for the other non-supported platforms.

@ShunLiu-NOAA ShunLiu-NOAA requested a review from hu5970 July 3, 2023 13:45
@ShunLiu-NOAA
Copy link
Contributor

@hu5970 Please review the modifications and let's consider the new development in dev-sci.

@MatthewPyle-NOAA
Copy link
Contributor

@MatthewPyle-NOAA, could you give the code owners (listed in .github/CODEOWNERS) the write access to this repository? Then, they will be set as the default PR reviewers when a new PR is submitted. I can see the error message This CODEOWNERS file contains errors. This is because they don't have the write access. If you think the file (.github/CONDEOWNERS) needs to be modified, please let me know.

@chan-hoo I'll try to get this sorted out

Copy link
Contributor

@MatthewPyle-NOAA MatthewPyle-NOAA left a comment

Choose a reason for hiding this comment

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

@chan-hoo Changes look good to me. I left a few comments, mostly about stuff that I think needs to be removed (JEDI, metplus, etc.), but maybe not critical to fix at this stage.

Copy link
Contributor

@MatthewPyle-NOAA MatthewPyle-NOAA left a comment

Choose a reason for hiding this comment

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

No objections from me from looking at changes...but hopefully Ben Blake can weigh in after more testing.

@BenjaminBlake-NOAA
Copy link
Contributor

@MatthewPyle-NOAA @chan-hoo I can definitely look over all these changes more thoroughly tomorrow morning. The build process worked well for me on WCOSS2 and Hera. Once I get access to the wrfruc project on Jet I will test building the code there and submit my final approval. Hopefully that won't take more than a day or two!

@chan-hoo chan-hoo added the bug Something isn't working label Jul 6, 2023
@chan-hoo
Copy link
Contributor Author

chan-hoo commented Jul 6, 2023

Sorry. Something wrong. I'll fix the dismiss and label issue.

@chan-hoo chan-hoo removed the bug Something isn't working label Jul 6, 2023
@chan-hoo
Copy link
Contributor Author

chan-hoo commented Jul 6, 2023

@MatthewPyle-NOAA, I don't know what happened here. I didn't dismiss your approval. Can you fix this?

Copy link
Contributor

@MatthewPyle-NOAA MatthewPyle-NOAA left a comment

Choose a reason for hiding this comment

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

Giving conditional approval, pending Ben Blake's testing.

@chan-hoo
Copy link
Contributor Author

chan-hoo commented Jul 6, 2023

Giving conditional approval, pending Ben Blake's testing.

Thank you!

@@ -3,7 +3,7 @@

# These owners will be the default owners for everything in the repo.
#* @defunkt
* @MatthewPyle-NOAA @ShunLiu-NOAA @JacobCarley-NOAA @chan-hoo @BenjaminBlake-NOAA @RatkoVasic-NOAA @JiliDong-NOAA @hu5970
* @NOAA-EMC/RRFS/RRFS-devs
Copy link
Contributor

Choose a reason for hiding this comment

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

I know some work was done to update the CODEOWNERS file, but GitHub is still saying the file contains errors. Should we revert back to the previous version which listed our individual usernames?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

devclean.sh Outdated
usage () {
cat << EOF_USAGE

Clean the UFS-SRW Application build
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest replacing UFS-SRW application with RRFS here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


OPTIONS:
PLATFORM - name of machine you are building on
(e.g. cheyenne | hera | jet | orion | wcoss2 )
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the reference to Cheyenne.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


OPTIONS:
PLATFORM - name of machine you are building on
(e.g. cheyenne | hera | jet | orion | wcoss2 )
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also remove this reference to Cheyenne.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


OPTIONS:
PLATFORM - name of machine you are on
(e.g. cheyenne | hera | jet | orion | wcoss2 )
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove Cheyenne as an option here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@BenjaminBlake-NOAA BenjaminBlake-NOAA left a comment

Choose a reason for hiding this comment

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

@chan-hoo I've finished reviewing all of the changes, they look great! I have a couple of general comments which I will leave here, and I added a few other suggestions to specific files. I am not going to formally submit my approval just yet but I will do that once I get access to Jet and can build the workflow there.

  • I agree with Matt about removing JEDIENVAR_IODA and RUN_PYTHON_GRAPHICS, but that can be done later.
  • Once we start running/testing the workflow and jobs, we should change python_srw to python_rrfs and SRW_ENV to RRFS_ENV in modulefiles/tasks. But I think that change is outside the scope of this PR.

@MatthewPyle-NOAA
Copy link
Contributor

Are you ready for this to be merged, @chan-hoo ?

@chan-hoo
Copy link
Contributor Author

chan-hoo commented Jul 7, 2023

@MatthewPyle-NOAA, whenever I submit a new commit, the existing reviewer's approvals are dismissed. I think we need to change some settings.

@chan-hoo
Copy link
Contributor Author

chan-hoo commented Jul 7, 2023

I found out your approval was only dismissed by a new commit. I think the code owner' (new) review is required for any new commits.

@MatthewPyle-NOAA
Copy link
Contributor

@chan-hoo I found this issue, but don't see the option described to stop the dismissal of reviews. I'll reapprove.

Copy link
Contributor

@MatthewPyle-NOAA MatthewPyle-NOAA left a comment

Choose a reason for hiding this comment

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

Giving another approval...

@chan-hoo
Copy link
Contributor Author

chan-hoo commented Jul 7, 2023

Thanks, I think it is ready to be merged now.

@MatthewPyle-NOAA MatthewPyle-NOAA merged commit 0f4df16 into NOAA-EMC:dev-sci Jul 7, 2023
@chan-hoo chan-hoo deleted the bugfix/dev_sci branch July 8, 2023 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants