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 DMI Cray XC50 hpc (freya) to machines. Based on Banting #528

Merged
merged 3 commits into from
Nov 3, 2020
Merged

Add DMI Cray XC50 hpc (freya) to machines. Based on Banting #528

merged 3 commits into from
Nov 3, 2020

Conversation

TillRasmussen
Copy link
Contributor

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

  • Short (1 sentence) summary of your PR:
    This adds environment and Macros for DMI Cray XC50 computer to machines. Based on Banting XC50.
  • Developer(s):
    tar
  • Suggest PR reviewers from list in the column to the right.
    @phil-blain @apcraig
  • Please copy the PR test results link or provide a summary of testing completed below.
    Do not change the code itself
  • How much do the PR code changes differ from the unmodified code?
    • [x ] bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • [x ] No
  • Does this PR add any new test cases?
    • Yes
    • [x ] No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
    • Yes
    • [ x] No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • [ x] Please provide any additional information or relevant details below:
    For DMI runs big endian flag should be removed.
    Big endian flag only in Macro to use test examples from CICE consortium.
    Only implemented for intel compiler

phil-blain
phil-blain reviewed Nov 2, 2020
View reviewed changes
configuration/scripts/cice.batch.csh Outdated
@@ -185,7 +185,7 @@ cat >> ${jobfile} << EOFB
#SBATCH --qos=standby
EOFB

else if (${ICE_MACHINE} =~ daley* || ${ICE_MACHINE} =~ banting*) then
Copy link
Member

Choose a reason for hiding this comment

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

Hi Till! I don't really mind that you add your machine here to the same else if statement as ours (the PBS directives should not change often), but if you (or I) want to change them in the future, maybe it makes more sense for you to have a separate entry for your machine ?

configuration/scripts/cice.launch.csh Outdated
@@ -130,7 +130,7 @@ EOFR
endif

#=======
else if (${ICE_MACHINE} =~ daley* || ${ICE_MACHINE} =~ banting*) then
Copy link
Member

Choose a reason for hiding this comment

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

same comment here. in fact I'm realizing that this is wrong for the serial case, as if we don't use aprun the model will run on the MOM node instead of the computes...

configuration/scripts/machines/Macros.freya_intel
CXX := CC
CPP := /usr/bin/cpp
CPPFLAGS := -P -traditional # ALLOW fortran double backslash "\\"
SCC := gcc
Copy link
Member

Choose a reason for hiding this comment

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

any reason you use gcc instead of cc for the serial case ?

configuration/scripts/machines/Macros.freya_intel Outdated

# Additional flags

ifeq ($(DITTO), yes)
Copy link
Member

Choose a reason for hiding this comment

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

the is probably obsolete.. greping either 'DITTO' and 'REPRODUCIBLE' does not return any result for me..

configuration/scripts/machines/Macros.freya_intel
# CPPDEFS := $(CPPDEFS) -Dncdf
#endif

### if using parallel I/O, load all 3 libraries. PIO must be first!
Copy link
Member

Choose a reason for hiding this comment

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

this part should have been removed long ago from our Macro files, PIO is not installed on our machines... So I would suggest removing it also here, to not carry uneeded stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gcc: The short answer makdep fails if I use cc. It works when I use gcc. I think, but I am not sure, that the reason is that the XC50 computer at DMI consist of login nodes and compute nodes, which have different architectures. The cc command is used when compiling for compute nodes and gcc for login nodes.
I will separate the computers within the if statement and update.
I agree about the DITTO and REPRODUCIBLE but I left them in there to be sure.
There is an installation of parallel netcdf on the DMI computer but I am not sure if it works. I have never tried CICE or other model systems with pio.

Copy link
Member

Choose a reason for hiding this comment

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

ah yes, makes sense to use gcc if the login and computes have different architectures. I think you also could use the CFLAGS_HOST macro for that : https://cice-consortium-cice.readthedocs.io/en/latest/user_guide/ug_running.html#cross-compiling

This would be a little "cleaner" I think, since this way the C code (ice_reprosum..) would still be compiled by cc (I don't know if it would really make a difference though.)

@TillRasmussen
Copy link
Contributor Author

I have now separated the two machines (Bainting and Freya) in cice.batch.csh and cice.launch.csh

apcraig
apcraig approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

Some effort was made to clean up the Macros a few months ago. They are now documented, https://cice-consortium-cice.readthedocs.io/en/master/user_guide/ug_case_settings.html#table-of-c-preprocessor-cpp-macros. I agree that DITTO and REPRODUCIBLE should be removed at this point, although it's not a big deal if they're not. I think they were removed from several other machine files at that time (although maybe not all).

In general, I think it's fine to share machine settings in the "if" tests for multiple machines. It's pretty easy to split them up later if needed. I agree with @phil-blain's comment that if multiple machine are sharing an "if" section, care must be taken if something needs to change for one machine.

@apcraig
Copy link
Contributor

apcraig commented Nov 3, 2020

@TillRasmussen, I'll give you a chance to make any additional revisions. Once you feel you're ready to merge (and maybe that's now), just add a comment to the PR or email me if you prefer. thanks!

@TillRasmussen
Copy link
Contributor Author

I added option for gnu as well for DMI Freya. I am done with my changes. These can be merged

@apcraig apcraig merged commit d95a4f3 into CICE-Consortium:master Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-blain phil-blain phil-blain left review comments

@eclare108213 eclare108213 eclare108213 approved these changes

@apcraig apcraig apcraig approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@TillRasmussen @apcraig @eclare108213 @phil-blain