Chemistry preprocessor compiler fixes for Cori #3045

cameronsmith1 · 2019-07-06T17:53:35Z

The chemistry pre-processor would not compile and run correctly on Cori. It was previously configured to work on Titan.

This PR fixes the compiler and CPP options so that it will work on Cori.

cpp is used by the chemistry pre-processor to manipulate the fortran output. It seems that modern versions of cpp behave differently than older versions, so the chemistry pre-processor has problems and produces different fortran output. There are two main issues:

cpp adds its copyright information to the top of the output using
C-type comments, which confuses the chemistry pre-processor (and
probably the E3SM Fortran compiler).
cpp deletes parts of the fortran output because it misidentifies it
as a comment.

Issue 1 occurs with the -C option to cpp, and issue 2 occurs without
the -C option to cpp. This commit solves the problem by replacing the
-C option with -traditional-cpp.

For future convenience, I copied pp_linoz_mam4_resus_mom_soag.inp from the pre-processed directory in E3SM to the chemistry pre-processor input directory (and added the header so that it will work with the standalone pre-processor). Note: linoz_mam4_resus_mom_soag is the standard chemistry configuration for E3SMv1.

[E3SM is BFB. Output of chemistry pre-processor on Cori is now the same as it was on Titan.]

[BFB]

cameronsmith1 · 2019-07-06T18:05:24Z

The one issue I know about so far, is that recreating the mechanism linoz_mam4_resus_mom_soag creates different code:

diff ./mo_imp_sol.F90 ../../../src/chemistry/pp_linoz_mam4_resus_mom_soag/mo_imp_sol.F90
297,298c297,298
<        call addfld( trim(solsym(j))
<        call addfld( trim(solsym(j))
---
>        call addfld( trim(solsym(j))//'_CHMP', (/ 'lev' /), 'I', '/cm3/s', 'chemical production rate' )
>        call addfld( trim(solsym(j))//'_CHML', (/ 'lev' /), 'I', '/cm3/s', 'chemical loss rate' )
658,659c658,659
<        call outfld( trim(solsym(j))
<        call outfld( trim(solsym(j))
---
>        call outfld( trim(solsym(j))//'_CHMP', prod_out(:,:,i), ncol, lchnk )
>        call outfld( trim(solsym(j))//'_CHML', loss_out(:,:,i), ncol, lchnk )

susburrows · 2019-07-10T17:37:56Z

Hi @cameronsmith1 , are there specific aspects of this that we need to review, e.g., whether one of us can reproduce the build on cori?

cameronsmith1 · 2019-07-11T01:57:16Z

I think the big question is: why is there a discrepancy in the fortran produced now versus previously?

Is it because we are on a different machine?

Did a change I made for this PR break something?

Did someone else change something?

rljacob · 2019-07-11T17:40:28Z

Is there any config that tests the chemistry?

susburrows · 2019-07-12T17:11:44Z

@rljacob The linoz_mam4_resus_mom_soag mechanism is the default atmospheric chemistry mechanism, so it is exercised by all of the tests that have an active EAM atmosphere component.

singhbalwinder · 2019-07-12T22:39:55Z

@cameronsmith1 : Would you please outline the steps you are using to create these files? There are two ways:

Use the make file to compile the model and then run the executable with an input file
Build a case (using create_newcase) and run just the ./case.setup with the following additional arguments to CAM_CONFIG_OPTS:
-usr_mech_infile /path/to/user_supplied_mech.in

I used the 2nd way and I got the files which were exactly the same as in pp_linoz_mam4_resus_mom_soag folder except that all new files have a disclaimer on the top and few extra new lines (empty lines). Specifically, I didn't see differences you are seeing in mo_imp_sol.F90.

I haven't tried the 1st method yet.

cameronsmith1 · 2019-07-13T06:37:36Z

I was using method 1 to build the files. This makes it easy to generate the files that need to be put into pp_* directory.

I thought that method 2 didn't work anymore (I am glad it does). The disclaimer caused all sorts of errors when creating the output code using method 1, and the header is a C header, so I expect it will fail to compile as part of E3SM.

As for tests, the previously generated output from the pre-processor that is archived in the PP_* directory is tested, but there is no test that makes sure the pre-processor will compile and still build the existing output code (AFAIK).

cpp is used by the chemistry pre-processor to manipulate the fortran output. It seems that modern versions of cpp behave differently, so the chemistry pre-processor has problems and produces different fortran output. There are two main issues: 1) cpp adds its copyright information to the top of the output using C-type comments, which confuses the chemistry pre-processor (and probably the E3SM Fortran compiler). 2) cpp deletes parts of the fortran output because it misidentifies it as a comment. Issue 1 occurs with the -C option to cpp, and issue 2 occurs without the -C option to cpp. This commit solves the problem by replacing the -C option with -traditional-cpp. [E3SM is BFB. Output of chemistry pre-processor is changed]

cameronsmith1 · 2019-07-15T22:33:05Z

I did a new commit to change the cpp flags, and that seems to have fixed the problem. Specifically, I replaced -C with -traditional-cpp

Can someone test that this works on other machines? Specifically, I have been checking that running the chemistry pre-compiler on components/cam/chem_proc/inputs/pp_linoz_mam4_resus_mom_soag.inp will produce the fortran output stored in components/cam/src/chemistry/pp_linoz_mam4_resus_mom_soag/

cameronsmith1 · 2019-07-22T23:43:37Z

What do we need to do to move forward on merging this to master?

singhbalwinder · 2019-07-23T17:10:28Z

Thanks @cameronsmith1 for figuring this out. I will test it on my end on a different machine. I am good with changes in mozzpp.main.f file. The other files seem machine/compiler specific. Do you think we should integrate those? Also, I don't think we should integrate the chemistry input file. I am assuming that you used the input file just for testing purposes.

cameronsmith1 · 2019-07-23T18:55:54Z

Thanks for testing on other systems. In a perfect universe we should have something that works on every machine. However, at a minimum we have should have something that works on at least one machine -- that was Titan, and now I think it should be Cori. Hence, I think it is critical we put in my Cori specific changes, unless there is something that is more universal.

The pre-processor input directory is supposed to contain all the input files, so I think it is a major oversight not to have the input file for the main E3SMv1 chemistry there. This PR just corrects the oversight. It is also critical when developing new mechanisms to have an input file to work from, and this provides that.

singhbalwinder · 2019-07-23T21:01:33Z

I have tested this on Compy with the Intel compiler. I had to set setenv COMPILER ifort variable in my environment to compile it with Intel (otherwise it was picking up the defaults). The use of traditional-cpp removed the GNU disclaimer from the files and the files I got were exactly the same as in pp_linoz_mam4_resus_mom_soag (except few additional white spaces and lines).

Therefore the code seems to be working as expected on Compy if we replace the -C flag with traditional-cpp (to get rid of the disclaimer). I think we should add this change and also add the remaining files as you mentioned.

cameronsmith1 · 2019-07-23T21:21:06Z

That is good news. Thanks, @singhbalwinder.

There are other compiler options configurations commented out in make_chempp.cori. These are options that have worked in the past, and I think they are worth keeping as options for the next time we need to get the pre-processor working. Does uncommenting the intel option work on compy? If so, I suggest we add a comment in the file to note that it works on Compy. If not, is there a tweak that will implement what you did?

singhbalwinder · 2019-07-25T23:18:01Z

Does uncommenting the intel option work on compy?

If you are referring to the changing make_chempp.cori file above, it won't work on Compy as Compy doesn't have PrgEnv-* modules. I think if we want to compile this code on a new (or different) machine we will have to load a compiler and run make on that machine. This tool needs just a compiler, so it is fairly easy to build.

I tested it further on Cori and was not able to reproduce your original error (call outfld( trim(solsym(j) .....). The code was behaving as expected if I just remove -C' flag. We don't even need to add -traditional-cpp or -traditional flag.

singhbalwinder · 2019-07-25T23:19:21Z

components/cam/chem_proc/src/Makefile

@@ -140,11 +140,15 @@ ifeq ($(COMPILER),xlf95)
  FFLAGS      := -g -c -qarch=auto -qnosave -qfree=f90 -qmoddir=$(OBJ_DIR) -I $(OBJ_DIR) -qstrict -O3
 endif
 ifeq ($(COMPILER),gfortran)
-  FFLAGS      := -g -c -ffree-form
+# FFLAGS      := -g -c -ffree-form
+  FFLAGS      := -g -c -ffree-form -J $(OBJ_DIR)


I do not know about this -J flag. The code works without this new addition. Is there a reason this was added?

The -J flag controls where the intermediate compiled files go. Without the -J flag they clogged up the src directory. The -J flag puts them in a separate directory $(OBJ_DIR), which I think has been the historical place for them. Hence, it is not strictly necessary, but it is valuable and increases consistency across different compilers without any downside. Hence, I recommend we keep it in the PR.

Thanks! It works for gfortran but it doesn't do anything if we use Intel. Line 143 above (# FFLAGS := -g -c -ffree-form) is not doing anything. Is it okay if I remove it?

Yes, you can remove the commented line. I was keeping it while I was testing new options. Since I ended up just adding an extra option (-J) there seems little value in keeping the original line around. Can you remove it? Or do you need me to do it?

I just removed the commented out line, for clarity.

singhbalwinder · 2019-07-25T23:24:21Z

components/cam/chem_proc/src/cam_chempp/mozpp.main.f

@@ -173,7 +173,8 @@ program mozart_pp
      sim_dat_filename = 'sim.dat'
      sim_dat_filespec = trim(sim_dat_path) // 'sim.dat'
      cpp_dir  = ' '
-      cpp_opts = '-P -C -I.'
+!     cpp_opts = '-P -C -I.'  ! -C causes problems on Cori
+      cpp_opts = '-P -I. -traditional-cpp'  ! Works on Cori (2019-07-15)


I think we can just remove the -C flag. Also, we should just get rid of the commented-out line.

Did you diff the fortran files to those archived in src/chemistry ?

My personal preference is to leave in (as comments) things that may be relevant to getting the pre-processor to work an another machine. It is a cheap alternative to IF statements for different machines.

Agreed. Lets keep it the way it is.

singhbalwinder · 2019-07-26T17:57:21Z

I am fine with the code in its current form. It is being tested on Compy and Cori and it works as expected with these new changes (thanks @cameronsmith1 !).

We should add a test for this code. It might be a good idea to add that test when we add tests for other CAM tools.

cameronsmith1 · 2019-07-31T02:01:47Z

I just tested this PR after the recent Cori upgrade, and it still compiles and produces the expected code output (BFB).

What do we need to do to finalize this PR?

Adding a test for the pre-processor would be good, but do we need to hold this PR for that?

singhbalwinder · 2019-07-31T04:02:38Z

Thanks Philip! I will make that change and merge this to next and master.

cameronsmith1 · 2019-08-02T20:32:29Z

I am back from jury duty. How is this going?

When modifying the compiler flags, a line was copied and modified, with the original version of the line left in, but commented out, for reference. This commit removes the old line to clean up the code. [BFB]

cameronsmith1 · 2019-08-05T21:13:11Z

I just pushed a commit to remove the commented-out in the Makefile, that we discussed.

I think this is now ready for merging to next, then master.

singhbalwinder · 2019-08-05T22:34:26Z

Thanks @cameronsmith1 ! There is another big atm PR being integrated right now. I will wait for that to merge to master before merging this one.

Chemistry preprocessor compiler fixes for Cori The chemistry pre-processor would not compile and run correctly on Cori. It was previously configured to work on Titan. This PR fixes the compiler and CPP options so that it will work on Cori. cpp is used by the chemistry pre-processor to manipulate the fortran output. It seems that modern versions of cpp behave differently than older versions, so the chemistry pre-processor has problems and produces different fortran output. There are two main issues: cpp adds its copyright information to the top of the output using C-type comments, which confuses the chemistry pre-processor (and probably the E3SM Fortran compiler). cpp deletes parts of the fortran output because it misidentifies it as a comment. Issue 1 occurs with the -C option to cpp, and issue 2 occurs without the -C option to cpp. This commit solves the problem by replacing the -C option with -traditional-cpp. For future convenience, I copied pp_linoz_mam4_resus_mom_soag.inp from the pre-processed directory in E3SM to the chemistry pre-processor input directory (and added the header so that it will work with the standalone pre-processor). Note: linoz_mam4_resus_mom_soag is the standard chemistry configuration for E3SMv1. [E3SM is BFB. Output of chemistry pre-processor on Cori is now the same as it was on Titan.] [BFB] * cameronsmith1/atm/chem_preproc_cori: Removed modified line that was commented out in chem-preproc Makefile Changed cpp options to avoid incorrect fortran output. Chemistry preprocessor compiler fixes for Cori

singhbalwinder · 2019-08-07T19:04:23Z

Merged to next

Chemistry preprocessor compiler fixes for Cori The chemistry pre-processor would not compile and run correctly on Cori. It was previously configured to work on Titan. This PR fixes the compiler and CPP options so that it will work on Cori. cpp is used by the chemistry pre-processor to manipulate the fortran output. It seems that modern versions of cpp behave differently than older versions, so the chemistry pre-processor has problems and produces different fortran output. There are two main issues: cpp adds its copyright information to the top of the output using C-type comments, which confuses the chemistry pre-processor (and probably the E3SM Fortran compiler). cpp deletes parts of the fortran output because it misidentifies it as a comment. Issue 1 occurs with the -C option to cpp, and issue 2 occurs without the -C option to cpp. This commit solves the problem by replacing the -C option with -traditional-cpp. For future convenience, I copied pp_linoz_mam4_resus_mom_soag.inp from the pre-processed directory in E3SM to the chemistry pre-processor input directory (and added the header so that it will work with the standalone pre-processor). Note: linoz_mam4_resus_mom_soag is the standard chemistry configuration for E3SMv1. [E3SM is BFB. Output of chemistry pre-processor on Cori is now the same as it was on Titan.] [BFB] * cameronsmith1/atm/chem_preproc_cori: Removed modified line that was commented out in chem-preproc Makefile Changed cpp options to avoid incorrect fortran output. Chemistry preprocessor compiler fixes for Cori

cameronsmith1 · 2019-08-08T20:09:24Z

Thanks!

cameronsmith1 · 2019-08-15T21:57:36Z

Note: a problem arose with the NERSC modules soon after this PR was pushed to master. It has now been fixed by NERSC. See issue #3137 for details.

Chemistry preprocessor compiler fixes for Cori

dff48a5

[BFB]

cameronsmith1 assigned singhbalwinder Jul 6, 2019

cameronsmith1 requested a review from susburrows July 6, 2019 17:54

cameronsmith1 added BFB PR leaves answers BFB Chemistry Pre-Processor Cori labels Jul 6, 2019

rljacob requested a review from jgfouca July 8, 2019 05:01

rljacob added the Atmosphere label Jul 8, 2019

jgfouca removed their request for review July 11, 2019 17:38

singhbalwinder reviewed Jul 25, 2019

View reviewed changes

Removed modified line that was commented out in chem-preproc Makefile

83c8a96

When modifying the compiler flags, a line was copied and modified, with the original version of the line left in, but commented out, for reference. This commit removes the old line to clean up the code. [BFB]

singhbalwinder merged commit 83c8a96 into master Aug 8, 2019

cameronsmith1 mentioned this pull request Aug 15, 2019

Chemistry pre-processor cannot find libgfortran.so.5 at NERSC #3137

Closed

tangq mentioned this pull request Apr 3, 2020

usr_mech_infile option doesn't build chemistry preprocessor on cori #3536

Open

jgfouca pushed a commit that referenced this pull request Nov 7, 2024

Merge pull request #3045 from E3SM-Project/bartgol/eamxx/dev-docs

41a944c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chemistry preprocessor compiler fixes for Cori #3045

Chemistry preprocessor compiler fixes for Cori #3045

cameronsmith1 commented Jul 6, 2019 •

edited

Loading

cameronsmith1 commented Jul 6, 2019

susburrows commented Jul 10, 2019

cameronsmith1 commented Jul 11, 2019 •

edited

Loading

rljacob commented Jul 11, 2019

susburrows commented Jul 12, 2019

singhbalwinder commented Jul 12, 2019 •

edited

Loading

cameronsmith1 commented Jul 13, 2019

cameronsmith1 commented Jul 15, 2019

cameronsmith1 commented Jul 22, 2019

singhbalwinder commented Jul 23, 2019

cameronsmith1 commented Jul 23, 2019

singhbalwinder commented Jul 23, 2019

cameronsmith1 commented Jul 23, 2019

singhbalwinder commented Jul 25, 2019

singhbalwinder Jul 25, 2019

cameronsmith1 Jul 26, 2019

singhbalwinder Jul 26, 2019

cameronsmith1 Jul 31, 2019

cameronsmith1 Aug 5, 2019

singhbalwinder Jul 25, 2019

cameronsmith1 Jul 26, 2019

cameronsmith1 Jul 26, 2019

singhbalwinder Jul 26, 2019

singhbalwinder commented Jul 26, 2019

cameronsmith1 commented Jul 31, 2019 •

edited

Loading

singhbalwinder commented Jul 31, 2019

cameronsmith1 commented Aug 2, 2019

cameronsmith1 commented Aug 5, 2019

singhbalwinder commented Aug 5, 2019

singhbalwinder commented Aug 7, 2019

cameronsmith1 commented Aug 8, 2019

cameronsmith1 commented Aug 15, 2019

Chemistry preprocessor compiler fixes for Cori #3045

Chemistry preprocessor compiler fixes for Cori #3045

Conversation

cameronsmith1 commented Jul 6, 2019 • edited Loading

cameronsmith1 commented Jul 6, 2019

susburrows commented Jul 10, 2019

cameronsmith1 commented Jul 11, 2019 • edited Loading

rljacob commented Jul 11, 2019

susburrows commented Jul 12, 2019

singhbalwinder commented Jul 12, 2019 • edited Loading

cameronsmith1 commented Jul 13, 2019

cameronsmith1 commented Jul 15, 2019

cameronsmith1 commented Jul 22, 2019

singhbalwinder commented Jul 23, 2019

cameronsmith1 commented Jul 23, 2019

singhbalwinder commented Jul 23, 2019

cameronsmith1 commented Jul 23, 2019

singhbalwinder commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhbalwinder commented Jul 26, 2019

cameronsmith1 commented Jul 31, 2019 • edited Loading

singhbalwinder commented Jul 31, 2019

cameronsmith1 commented Aug 2, 2019

cameronsmith1 commented Aug 5, 2019

singhbalwinder commented Aug 5, 2019

singhbalwinder commented Aug 7, 2019

cameronsmith1 commented Aug 8, 2019

cameronsmith1 commented Aug 15, 2019

cameronsmith1 commented Jul 6, 2019 •

edited

Loading

cameronsmith1 commented Jul 11, 2019 •

edited

Loading

singhbalwinder commented Jul 12, 2019 •

edited

Loading

cameronsmith1 commented Jul 31, 2019 •

edited

Loading