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

Jgfouca/branch for acme split 2019 06 10 #3139

Merged
merged 58 commits into from
Jun 12, 2019

Conversation

jgfouca
Copy link
Contributor

@jgfouca jgfouca commented Jun 10, 2019

Changes:

Test suite: scripts_regression_tests
Test baseline:
Test namelist changes:
Test status: [bit for bit, roundoff, climate changing]

Fixes [CIME Github issue #]

User interface changes?:

Update gh-pages html (Y/N)?:

Code review:

jgfouca and others added 30 commits May 2, 2019 17:47
and cam is to just put all the sources into cam
[BFB] - Bit-For-Bit

See confluence for a more detailed description about these tags.
Added logic for compy to provenance.py, specified
the location of the performance archive in the
compy entry in config_machines.xml (enabled for
all projects) and added the job progress monitoring
script syslog.compy. Also removed performance
archiving support for edison, now that it has been
decommissioned.

[BFB] - Bit-For-Bit
* origin/master: (159 commits)
  Update CIME to ESMCI cime5.8.2-3 (PR #2938)
  Update namelist scripts to reflect changes in mpas-ocean Registry files
  Update mpas source.
  bless_test_results: Be able to handle build-only tests
  Fix machine file entry for C++ compiler on melvin.
  Add x2g_*_from_lnd lists for use in prep_glc LND routines
  Update config_machines.xml
  Fix optics allocation
  Cleanup formatting in compute_melt_fluxes()
  Remove no longer-used cime_model variable
  Fix unallocated cloud optics
  Fix unallocated band midpoints
  COMPOSE: Fix the scope of the share_kokkos_ut standalone unit tests.
  Update CIME to ESMCI cime5.8.2-2 (#2908)
  Minor cleanup to previous commit
  Add default to PIO_CLOBBER if mode unset
  Test with changing logic for ocn-glc coupling
  Fix formatting and whitespace
  Remove `mode_in` in calls to `cam_pio_createfile`
  Add subroutine call to get PIO format
  ...
Need to use build as a library, not a subprocess.

[BFB]
Need to use the base case id when parsing test opts.

[BFB]
Adding an FC test with theta SL

Adding a short test for coupled run with theta-l and SL.

[BFB]

* oksanaguba/eam/fc-test-thetasl:
  addign the test
oksanaguba and others added 3 commits June 6, 2019 17:03
Add coupled dycore tests using shared execs

Add tests for coupling mechanisms in EAM and for SL transport with
theta dycore for coupled atmosphere. Tests re-use executables; only 2
builds are required, one for preqx dycore and another one for theta.

[Non-BFB] because new baselines are required.

* origin/oksanaguba/eam/tests-ftypes2:
  reducing dtime for all theta + ne4 tests, still need to try it on skybridge
  moving tests to integration
  change hires settings to lowres
  moving thetasl test to shared execs set
  start over with tests
* esmci_remote_for_split/master: (1734 commits)
  Update for cime5.8.3
  update esmf nuopc build on cheyenne
  fix nuopc build, update for stamepede esmf lib
  fixes for nuopc scripts_regressions_tests to work
  do not move mediator build
  Add logic to control activation of glcshelf_c2_ice
  fix py3 issue
  fix nuopc K_TestCimeCase
  rework nuopc build
  updates for nuopc on stampede
  fix build issues for nuopc, add dir to .gitignore
  Use idmap_ignore for optional mapping files
  revert change
  fix indentation
  Move MAP xml variables into shared config_component.xml
  make test optional
  Cleanup whitespace / indentation
  move Externals.cfg to ../Externals_cime.cfg (UFSCOMP)
  Minor refactor of kokkos stuff in build.py
  addition uf usermods documentation
  ...

# Conflicts:
#	scripts/lib/CIME/build.py
#	src/components/data_comps/datm/cime_config/namelist_definition_datm.xml
#	src/drivers/mct/main/cime_comp_mod.F90
@jgfouca jgfouca requested a review from jedwards4b June 10, 2019 21:32
@jgfouca jgfouca self-assigned this Jun 10, 2019
@@ -511,6 +503,38 @@ using a fortran linker.
<TRILINOS_PATH MPILIB="mpich2">$ENV{TRILINOS_PATH}</TRILINOS_PATH>
</compiler>

<compiler MACH="athena">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These additions are suspect.

Copy link
Contributor

Choose a reason for hiding this comment

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

seem to be a repeat of lines starting at 474

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, i will remove

jgfouca added 5 commits June 10, 2019 16:17
…-threads-for-mpi-only-runs"

This reverts commit 89fad14, reversing
changes made to 5c43a15.
So test drivers need to be updated to pass Cmake data as arguments.
@billsacks
Copy link
Member

@jgfouca with this change

Change FFLAGS_NOPT to be an append to the normal set of flags. This reduces duplication with FFLAGS

What is the purpose of FFLAGS_NOOPT as compared with <append DEBUG="TRUE">? It seems that the two are now accomplishing the same thing, so it seems confusing to have both of them.

Also, have you or someone else verified that the resulting flags for a debug build of cesm are unchanged for all compilers for which we have FFLAGS_NOOPT defined? If not yet, I'd like to request that that comparison be done before this PR is merged to master, to avoid any surprises.

@billsacks billsacks self-requested a review June 11, 2019 20:08
@billsacks
Copy link
Member

(Adding myself as a reviewer so that I can mark when I feel my above comment has been addressed.)

@jgfouca
Copy link
Contributor Author

jgfouca commented Jun 11, 2019

What is the purpose of FFLAGS_NOOPT as compared with ? It seems that the two are now accomplishing the same thing, so it seems confusing to have both of them.

@billsacks , that's a good point, they are very similar now. I think the only difference is that FFLAGS_NOOPT guarantees that the flags come later in the compile line than the normal FFLAGS. EDIT: they are actually different concepts: DEBUG is set to true when user requests a debug build, but we have certain special files that always disable optimizations because they would take too long to compile.

I did confirm that the builds we as expected for us in cam (using CMake). I used the bld_diff tool to validate.

@jgfouca
Copy link
Contributor Author

jgfouca commented Jun 11, 2019

@billsacks , as an example of a NOOPT file:

rrtmg_lw_k_g.o: rrtmg_lw_k_g.f90
        $(FC) -c $(FPPFLAGS) $(INCLDIR) $(INCS) $(FREEFLAGS) $(FFLAGS) $(FFLAGS_NOOPT) $<

rrtmg_sw_k_g.o: rrtmg_sw_k_g.f90
        $(FC) -c $(FPPFLAGS) $(INCLDIR) $(INCS) $(FREEFLAGS) $(FLLAGS) $(FFLAGS_NOOPT) $<

@billsacks
Copy link
Member

@jgfouca Thanks a lot for the bulleted summary of the major changes in here. In the future, for cime splits like this that include a lot of different changes that potentially impact CESM as well as E3SM (i.e., a lot more than just e3sm-specific config changes), it would help if you could also include pointers to the E3SM PR associated with each bulleted item, to make it easier for us to review particular changes separately. I have done that this time by editing your initial comment (hope you don't mind).

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

@jgfouca thanks for your clarification about FFLAGS_NOOPT. However, now I'm confused by what did and didn't change here. Given that you changed the definition of FFLAGS_NOOPT in config_compilers.xml, I expected to see all references to $(FFLAGS_NOOPT) changed to $(FFLAGS) $(FFLAGS_NOOPT). I see this in the Makefile, but not in the various Depends files in cime/config/machines/Depends.*. Am I missing something?

Also, it seems like this change will change behavior for CESM with intel: files compiled with FFLAGS_NOOPT will now have all of -qno-opt-dynamic-align -convert big_endian -assume byterecl -ftz -traceback -assume realloc_lhs -fp-model source in the compile line, followed by -O0. @jedwards4b is this acceptable / desirable?

Also see one inline comment about a typo.

That said, I do like this cleanup in general.


rrtmg_sw_k_g.o: rrtmg_sw_k_g.f90
$(FC) -c $(FPPFLAGS) $(INCLDIR) $(INCS) $(FREEFLAGS) $(FFLAGS_NOOPT) $<
$(FC) -c $(FPPFLAGS) $(INCLDIR) $(INCS) $(FREEFLAGS) $(FLLAGS) $(FFLAGS_NOOPT) $<
Copy link
Member

Choose a reason for hiding this comment

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

Typo: FLLAGS should be FFLAGS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Not sure how that made it through my verification...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wasn't using the Makefile to build those (they are in cam, which switched to CMake). And the baselines were generated with master.

@jgfouca
Copy link
Contributor Author

jgfouca commented Jun 11, 2019

@billsacks , you're right, I neglected to update some of the Make Depends files. Doing that now.

@jgfouca
Copy link
Contributor Author

jgfouca commented Jun 11, 2019

OK, all tests are passing for me now, so I'll merge once you guys approve.

@billsacks
Copy link
Member

@jgfouca thanks for these fixes. This looks fine to me, now, but I'd still like @jedwards4b to confirm the change I called out in #3139 (review)

@jedwards4b
Copy link
Contributor

@billsacks I don't think that it will be a problem, overall I think that the cleanup is worth the risk.

@jgfouca
Copy link
Contributor Author

jgfouca commented Jun 12, 2019

Thanks, @billsacks !

@jgfouca jgfouca merged commit a3d36d3 into master Jun 12, 2019
@jgfouca jgfouca deleted the jgfouca/branch-for-acme-split-2019-06-10 branch June 12, 2019 18:03
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.

8 participants