Mesh conversion makefile improvements #158

pwolfram · 2017-04-03T20:39:20Z

Fixes compile issues on titan (#156)

pwolfram · 2017-04-03T20:43:01Z

@xylar and @akturner if either of you could just confirm that this branch works and merge it that would be great.

xylar · 2017-04-03T20:59:52Z

grid_gen/mesh_conversion_tools/Makefile

@pwolfram, I think -lstdc++ is being included redundantly in this case.

xylar · 2017-04-03T21:00:27Z

grid_gen/mesh_conversion_tools/Makefile

Isn't this else case if $NETCDF is undefined? If so, does this line make sense?

Please take another look post-indentation fixing.

xylar · 2017-04-03T21:01:33Z

grid_gen/mesh_conversion_tools/Makefile

Same as two lines above.

This was confusing because of the inundation for the if-elseif-else etc statements. Please take another look now that indentation is fixed.

Ah, indeed. Sorry for the confusion on my part.

It was my fault and confused me too!

xylar · 2017-04-03T21:03:04Z

grid_gen/mesh_conversion_tools/Makefile

No reason to change the order of these 3, right? I assume that was for debugging purposes but might as well switch it back so we can more easily tell what actually changed.

xylar · 2017-04-03T21:03:51Z

grid_gen/mesh_conversion_tools/mpas_mask_creator.cpp

Nice, this was causing a lot of unnecessary trouble.

xylar · 2017-04-03T21:06:39Z

grid_gen/mesh_conversion_tools/mpas_mask_creator.cpp

I don't believe this change is correct. The original code was nullifying the whole string, which is good to do. Just adding a null character at the beginning is not necessarily good enough depending on how the char array is manipulated later. I would suggest reverting this change.

xylar · 2017-04-03T21:08:23Z

I think someone from each MPAS component that uses these tools needs to make sure they work as expected before we can merge. I have added @mark-petersen and @matthewhoffman as reviewers for this reason.

* Determines appropriate version of netcdf to use * Minor compatability fixes * adds Titan instructions

matthewhoffman · 2017-04-04T19:16:47Z

@xylar , to be clear, I should just confirm that I can build and use these tools on the platforms on which we normally work?

xylar · 2017-04-04T19:34:23Z

@matthewhoffman, yes, I think that's the highest priority. To make sure this PR does no harm to your normal workflow with these tools. @mark-petersen, same on the ocean side; @akturner, same on the sea-ice side.

matthewhoffman

I tested the changes on my Mac laptop and Linux desktop and had no problems with either. Looks good to me, and thanks for making these improvements.

mark-petersen · 2017-04-05T19:00:03Z

Summary: Tested on grizzly. gnu works. intel works on master but not on this PR.

gnu works
module load gcc/5.3.0 openmpi/1.10.5 netcdf/4.4.1 parallel-netcdf/1.5.0 pio/1.7.2
compiled using make in MPAS-Tools/grid_gen/mesh_conversion_tools
tested MPAS-O nightly regression suite with this version of MPAS-Tools, was bfb with previous.

intel does not
module load python/anaconda-2.7-climate;module load intel/17.0.1 openmpi/1.10.5 netcdf/4.4.1 parallel-netcdf/1.5.0 pio/1.7.2
changed Makefile to intel with these lines:

CC=icpc
CFLAGS= -O3 -std=c++0x -qopenmp -lstdc++
DFLAGS= -g -std=c++0x -D_DEBUG -qopenmp -lstdc++

and commenting gnu. This compiles and can successfully run on MPAS-Tools/master. On this branch, I get:

gr-fe3.lanl.gov> pwd
/turquoise/usr/projects/climate/mpeterse/repos/MPAS-Tools/pr_temp/grid_gen/mesh_conversion_tools
gr-fe3.lanl.gov> make clean
...
gr-fe3.lanl.gov> make
icpc mpas_mesh_converter.cpp netcdf_utils.cpp -O3 -std=c++0x -qopenmp -lstdc++ -o MpasMeshConverter.x -I. /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1 -I/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1 -L/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/lib -lnetcdf -lnetcdf_c++
/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1: file not recognized: Is a directory
make: *** [all] Error 1

It looks like a library problem, but it didn't happen on master, I'm not sure what is going on here.

mark-petersen · 2017-04-05T21:18:42Z

grid_gen/mesh_conversion_tools/Makefile

-# CFLAGS= -O3 -std=c++0x -openmp
-# DFLAGS= -g -std=c++0x -D_DEBUG -openmp
+# CFLAGS= -O3 -std=c++0x -openmp -lstdc++
+# DFLAGS= -g -std=c++0x -D_DEBUG -openmp -lstdc++


The intel flag has changed from -openmp to -qopenmp on these two lines. Right now we get:
icpc: command line remark #10411: option '-openmp' is deprecated and will be removed in a future release. Please use the replacement option '-qopenmp'

mark-petersen · 2017-04-05T21:24:55Z

grid_gen/mesh_conversion_tools/Makefile

+ifneq (${NETCDF}, )
+	ifneq ($(shell which ${NETCDF}/bin/nc-config 2> /dev/null), )
+		LIBS = $(shell ${NETCDF}/bin/nc-config --libs) -lnetcdf_c++
+		INCS = $(shell ${NETCDF}/bin/nc-config --cflags)


The previous two lines are causing the problem on grizzly using intel. For some reason I don't understand, I end up with the $NETCDF path prepended to both $LIBS and $INCS due to these lines. That is, I get:

gr-fe3.lanl.gov> pwd /turquoise/usr/projects/climate/mpeterse/repos/MPAS-Tools/pr_temp/grid_gen/mesh_conversion_tools gr-fe3.lanl.gov> module load intel/17.0.1 openmpi/1.10.5 netcdf/4.4.1 parallel-netcdf/1.5.0 pio/1.7.2 gr-fe3.lanl.gov> make icpc mpas_mesh_converter.cpp netcdf_utils.cpp -O3 -std=c++0x -openmp -lstdc++ -o MpasMeshConverter.x -I. /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1 -I/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1 -L/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/lib -lnetcdf -lnetcdf_c++

xylar · 2017-04-05T21:27:58Z

@mark-petersen, it looks like the ${INCS} variable is total nonsense, based on your command line. It seems to include the raw directory with no -I flag in addition to including it with the -I flag. Would be good to know what happens when you run each of the following when you have the intel libraries loaded:

echo $NETCDF
which ${NETCDF}/bin/nc-config
${NETCDF}/bin/nc-config --cflags

xylar · 2017-04-05T21:34:31Z

grid_gen/mesh_conversion_tools/Makefile

+else
+	LIBS= -L${NETCDF}/lib
+	LIBS += -lnetcdf_c++ -lnetcdf
+	INCS = -I${NETCDF}/include


@pwolfram, isn't $NETCDF empty in this case? So you might as well try not adding any path to LIBS and INCS and hope it's in the default path. Or issue an error.

Yes, I see. Based on your commands above, on grizzly with intel loaded:

gr-fe2.lanl.gov> ${NETCDF}/bin/nc-config --cflags /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1 -I/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include

That is the trouble. For some reason intel adds the extra path. Compare to the gnu version on grizzly:

gr-fe3.lanl.gov> ${NETCDF}/bin/nc-config --cflags -I/usr/projects/climate/lvanroekel/netcdf_gcc/include

In fact, it always does it. Look at this strange behavior on grizzly with intel:

gr-fe2.lanl.gov> module use /usr/projects/climate/SHARED_CLIMATE/modulefiles/all/;module load python/anaconda-2.7-climate;module load intel/17.0.1 openmpi/1.10.5 netcdf/4.4.1 parallel-netcdf/1.5.0 pio/1.7.2; gr-fe2.lanl.gov> ${NETCDF}/bin/nc-config --includedir /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1 /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include gr-fe2.lanl.gov> ${NETCDF}/bin/nc-config --libs /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1 -L/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/lib -lnetcdf

this appears to be a bug in the nc-config script. I guess we need a work-around.

vanroekel · 2017-04-05T22:12:58Z

@mark-petersen and @xylar this has to be an issue with how I installed the netcdf-c libraries. I've been meaning to correct the gnu one for a while, but have not had time to get back to this. What I don't understand is why the entry for --cflags in nc-config --all looks reasonable (below), but nc-config --cflags prepends garbage. Note, that nf-config --cflags gives the appropriate result as well. I will look into this. But my netcdf install directory is on scratch2 which is not responsive right now.

  --cc        -> icc
  --cflags    ->  -I/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include
  --libs      -> -L/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/lib -lnetcdf

  --has-c++   -> no
  --cxx       ->
  --has-c++4  -> no
  --cxx4      ->

  --fc        -> gfortran
  --fflags    -> -I/usr/projects/climate/lvanroekel/netcdf_gcc/include
  --flibs     -> -L/usr/projects/climate/lvanroekel/netcdf_gcc/lib -lnetcdff -L/usr/projects/climate/lvanroekel/netcdf_gcc/lib -lnetcdf -lnetcdf
  --has-f90   -> no

  --has-dap   -> no
  --has-nc2   -> yes
  --has-nc4   -> yes
  --has-hdf5  -> yes
  --has-hdf4  -> no
  --has-pnetcdf-> no

  --prefix    -> /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1
  --includedir-> /usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include
  --version   -> netCDF 4.3.3.1

vanroekel · 2017-04-06T04:51:36Z

@mark-petersen can you please try your tests again. I think I have fixed the grizzly issue. Here are @xylar's commands with what I now get on grizzly with intel/17.0.1

_echo $NETCDF_
/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1

_which ${NETCDF}/bin/nc-config_
/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/bin/nc-config

_${NETCDF}/bin/nc-config --cflags_
-I/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include

Also for gcc/5.3.0

_${NETCDF}/bin/nc-config --cflags_
-I/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/gcc-5.3.0/include

mark-petersen · 2017-04-06T19:26:19Z

Yes, @vanroekel that did it. Thanks, that is really great! I was able to successfully compile with intel on grizzly, and run an init_step1 with it.

Here you can see the calls that are now fixed:

gr-fe1.lanl.gov> module use /usr/projects/climate/SHARED_CLIMATE/modulefiles/all/;module load python/anaconda-2.7-climate;module load intel/17.0.1 openmpi/1.10.5 netcdf/4.4.1 parallel-netcdf/1.5.0 pio/1.7.2
gr-fe1.lanl.gov> ${NETCDF}/bin/nc-config --cflags
-I/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include
gr-fe1.lanl.gov> ${NETCDF}/bin/nc-config --includedir
/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/include
gr-fe1.lanl.gov> ${NETCDF}/bin/nc-config --libs
-L/usr/projects/climate/SHARED_CLIMATE/software/grizzly/netcdf/4.4.1/intel-17.0.1/lib -lnetcdf

mark-petersen · 2017-04-06T19:27:02Z

I also just pushed up one commit changing the updated intel flag name.

pwolfram · 2017-04-12T15:52:46Z

@mark-petersen, are we at the point where everyone should review this prior to a merge?

mark-petersen

I tested my standard process on grizzly with intel and gnu. After the above changes, everything works.

pwolfram · 2017-04-12T17:08:16Z

@akturner, thoughts on merging this PR that was used to address the compilation issue you found in the last week or so?

pwolfram · 2017-04-12T17:08:36Z

@xylar, it is probably good if you have another look at this too.

akturner

I dont have time to test this so go ahead with the merge

xylar · 2017-04-12T19:44:19Z

@pwolfram, since we have special instructions for Titan, it might be worth adding these for Edison:

#### edison instructions ####
# # In this file change:
# CC = CC
# # may need to unload parallel NetCDF and HDF5 libraries
# # to avoid g++ conflicts:
# module unload python
# module load cray-netcdf
# module load netcdf-cxx
# export NETCDF=/usr/common/graphics/netcdf-cxx/4.2
# make

Edit as you see fit.

xylar · 2017-04-12T19:47:27Z

I was able to build successfully on my laptop and on Wolf (both g++ and icpc) with the default settings. I was able to build on Edison following the above instructions.

@pwolfram, if you add the comment above (or if you tell me that you'd prefer not to), I'll merge.

pwolfram · 2017-04-12T20:11:23Z

@xylar, please feel free to add the comments for edison as a new commit and then merge. Thank you!

xylar · 2017-04-12T20:13:26Z

Will do.

pwolfram · 2017-04-12T20:25:36Z

Thanks @xylar!

pwolfram added bug enhancement labels Apr 3, 2017

pwolfram requested review from akturner and xylar April 3, 2017 20:39

pwolfram mentioned this pull request Apr 3, 2017

Cant install the mesh conversion tools on Titan #156

Closed

pwolfram force-pushed the mesh_conversion_makefile branch from 36f88ff to eaf6ced Compare April 3, 2017 20:42

pwolfram mentioned this pull request Apr 3, 2017

Generalize netcdf build systems #136

Closed

xylar reviewed Apr 3, 2017

View reviewed changes

grid_gen/mesh_conversion_tools/mpas_mask_creator.cpp Outdated

Copy link

Collaborator

xylar Apr 3, 2017

Choose a reason for hiding this comment

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

Nice, this was causing a lot of unnecessary trouble.

xylar reviewed Apr 3, 2017

View reviewed changes

xylar requested review from mark-petersen and matthewhoffman April 3, 2017 21:07

pwolfram force-pushed the mesh_conversion_makefile branch 2 times, most recently from baf579c to ded5e5f Compare April 3, 2017 21:42

Updates build process for mesh_converter

b7ff563

* Determines appropriate version of netcdf to use * Minor compatability fixes * adds Titan instructions

pwolfram force-pushed the mesh_conversion_makefile branch from ded5e5f to b7ff563 Compare April 3, 2017 21:43

matthewhoffman approved these changes Apr 4, 2017

View reviewed changes

mark-petersen reviewed Apr 5, 2017

View reviewed changes

xylar reviewed Apr 5, 2017

View reviewed changes

Change intel -openmp to -qopenmp

5dc141c

mark-petersen self-requested a review April 12, 2017 15:58

mark-petersen approved these changes Apr 12, 2017

View reviewed changes

akturner approved these changes Apr 12, 2017

View reviewed changes

xylar self-assigned this Apr 12, 2017

xylar approved these changes Apr 12, 2017

View reviewed changes

Add Edison instructions; change gfortran --> gnu

be52def

xylar merged commit be52def into MPAS-Dev:master Apr 12, 2017

xylar deleted the mesh_conversion_makefile branch April 12, 2017 20:22

Mesh conversion makefile improvements #158

Mesh conversion makefile improvements #158

Uh oh!

Conversation

pwolfram commented Apr 3, 2017

Uh oh!

pwolfram commented Apr 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Apr 3, 2017

Uh oh!

matthewhoffman commented Apr 4, 2017

Uh oh!

xylar commented Apr 4, 2017

Uh oh!

matthewhoffman left a comment

Choose a reason for hiding this comment

Uh oh!

mark-petersen commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanroekel commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanroekel commented Apr 6, 2017

Uh oh!

mark-petersen commented Apr 6, 2017

Uh oh!

mark-petersen commented Apr 6, 2017

Uh oh!

pwolfram commented Apr 12, 2017

Uh oh!

mark-petersen left a comment

Choose a reason for hiding this comment

Uh oh!

pwolfram commented Apr 12, 2017

Uh oh!

pwolfram commented Apr 12, 2017

Uh oh!

akturner left a comment

Choose a reason for hiding this comment

Uh oh!

xylar commented Apr 12, 2017

Uh oh!

xylar commented Apr 12, 2017

Uh oh!

vanroekel commented Apr 5, 2017 •

edited

Loading