fengyanshi / FUNWAVE-TVD

43 stars 51 forks source link

Modified makefile and command-line argument input #34

Closed zhoutengye closed 5 years ago

zhoutengye commented 5 years ago

The new makefile has the following features: (1) Makefile does not have to be in src, but can be everywhere (2) All the pre-processor code files, module files, and objective files will be well organized (3) The more simplified and efficient option list (4) Can work like the old Makefile when copy to src and set to correct path (5) All the old-version Makefiles are moved to "GNUMake" directory

The new command-line argument features: (1) when just execute "mpirun -np xx ./funwave" without argument, the executable will read from "input.txt" (2) when just execute "mpirun -np xx ./funwave foo" without argument, the executable will read from the file named "foo"(with extension)

malej commented 5 years ago

@fengyanshi @zhoutengye I see the direct benefit of the last two points, for specifying a different input file. Good stuff! However, I have issues with the overall Makefile overhaul. Has this been tested on multiple architectures, because none of the HPC DoD Makefiles work now (we don't just have openmpi or mpich; we have intel, sgi, etc.). I don't mean to be critical, but I am wondering what the premise behind this setup was? The original Makefile was rather simple and FUNWAVE does not rely on any external packages/libraries outside of MPI. Hence, everything was based on knowing which compiler wrapper one is using and where it sits. If you wanted to not list all the .F as being worried that a new module needs to be added all Makefiles separately, these can be overhauled easily to just grab all .F. Perhaps you can help me understand the current setup, because I am just finding it overly confusing. Suggest to revisit this pull request and establish a baseline need for automating this workflow before merging it into the master.

zhoutengye commented 5 years ago

Hi, @malej

Since I only have gnu and Intel compiler with openmpi/mpich, those are the only tested case.

In my opinion, the new makefile now works as an alternative of the old version makefile. It will gradually become the successor of old version Makefile. The old version makefile shall still work if the new Makefile does nor work. Right now, only gnu and Intel with mpich/openmpi tested, for different PC/HPC, there are two ways: (1) write a new Make_Essential file which includes the compiler and mpi information, and The makefile includes the specific Make_Essential. (2) customize the compiler and flags in the "uncommon" parts in Makefile.

As you have mentioned, None of the HPC DoD Makefile works. There are two possible causes: (1) The computational environment does not match the Makefile (Or the current makefile has not yet been tested the computational environment) (2) The function to read the command line argument varies with compilers, the GETARG function in IO.F works with gnu and intel, but may not work with all other compilers (which remains to be tested). I only added no more 10 lines to IO.F and that is the only modification to the source code. Please let me know if you can compile and run the FUNWAVE using the old version Makefile.

I am glad to discuss the details of the Makefile. If it is the new version Makefile is really not easy to use, we may still use the old makefiles and keep on testing the new version until it is ready to release.

Best regards, Zhouteng

malej commented 5 years ago

@zhoutengye - if this is something that @fengyanshi decided to go forward with and adopt, then that's fine. The point being though that if this has not been tested on all supported machines, then it should not be merged into master branch yet. The pull request should be done to another interim branch. If we adhere to basic software engineering practices, simplicity is the key. Hence, I am struggling with understanding how adding additional files (makefiles and such) is going to help with the compile process. What were you guys trying to fix/clean up by deciding to go with this flow? What is this new approach suppose to help with? It's just not clear to me, as debugging multiple nested makefiles is not easy for most users. Again, since FUNWAVE does not really have an array of external packages, I don't see the immediate benefit. Perhaps there is a reason for this change, I just need you to clearly explain what is the purpose behind this, when a simple clean up of current Makefile could do the job. You can certainly have a top-level Makefile, which even queries the system for its components (if you want to remove the user from the process some) and in turn that would call an appropriate Makefile in src directory, but when I look inside your makefile in essentials with all the if-statements, that is very error prone in the future. Just my two cents. Again, help me understand the reasons behind it, so I can support the effort if this is to be the direction we will go with.

fengyanshi commented 5 years ago

please hold on I will discuss with Zhouteng how to make this stuff more generic. Should give users an option they can use original makefiles.

mayhl commented 5 years ago

Hi @zhoutengye, @malej and @fengyanshi

If this Makefile can be modified for HPC use, I can see how it can be a bit easier for users; however, users will still have to play around with Makefile to add/remove specific modules, so it is not hiding a lot from the users. One benefit of having all the Makefiles is that it helps compiling FUNWAVE-TVD with different modules faster as you do not have to edit flags all the time, and user error can lead to compiling the incorrect version.

On the development side it would make things easier when new files are added/removed as there is no need to modify every Makefile, just the Make_Essentials file (an issue I came across before); however, some more detailed comments would help, as it is not obvious to tell what the if statements are doing at a quick glance.

Mike

fengyanshi commented 5 years ago

I just tried the new Makefile on topaz. It's cool. I felt the advantages as Mike just said. This approach is also consistent with those ocean community models such as FVCOM and ROMS. For users who like the original Makefiles, we can keep the folder 'Old_Version_Makefiles'. No problem.

malej commented 5 years ago

@fengyanshi so what exactly did you have to do to compile it? I didn't test on Topaz as that machine is being decommissioned in 3 weeks. I tried on Onyx. Is there a README file or something like that to know what command you want to invoke (what to type in the command line)?

fengyanshi commented 5 years ago

@malej Zhouteng is working on a brief documentation about how to use the makefiles. I told him to make .rst format so we can use it on the WIKI.

malej commented 5 years ago

@fengyanshi what did you change on Topaz (what file, where?) to make this compile. Topaz has an alias (mpif90) that point to ifort for example, but none of that on Onyx. On Onyx I had to change the programming environment from Cray to Intel, then (in the top level Makefile) DEF_FC=ftn, COMPILER=intel, removed -lmpi in CLIB, cleared the MPI= as no mpich or openmpi here, and turned on some flags (vessel, sediment, etc.). Then a long executable was built with the name: funwave-VESSEL-SEDIMENT-CHECK_MASS_CONSERVATION-TRACKING--intel-parallel-double in the funwave-work directory. In addition, all *.mod files were placed in the top level directory (not in src).

So, ultimately there are Makefiles in source, in top directory, in essential - why? What is the point of all this complexity? This can be a huge headache for a common user. I completely understand having good build systems for complex models like FVCOM, which rely on many external packages, but why FUNWAVE? You have 15 or so Fortran files that get pre-processed, then compiled, then linked without linking to any external packages/modules. All you had to do is change the FC and maybe MPI flag is -I, -L, and -l where required. Is there a plan to change into progressively larger use of external libraries? Can you guys explain to me why you changed this? I just don't get it. Simplicity is still key and people will appreciate it, as stripping away expert user level help will make folks adopt the model more.

mayhl commented 5 years ago

Hi @zhoutengye, @malej and @fengyanshi

The issue with the mod files being placed in root directory (or the directory of the Makefile) is due to the -J flag not being added with our Onyx input. Also, as far as I remember, the -J flag is not accepted by all compilers, so we have to be careful where .mod files are outputted.

If the single Makefile is the direction the repo is going with, then I think that we should just adapted the Make_Essentials to handle HPCs and toss away the old Makefiles. The old Makefiles would complicate things for non-advanced users and cause divergence in the repo.

Some suggestions to the current system:

Mike

fengyanshi commented 5 years ago

I just talked with Zhouteng. He is working on documentation on how to use/modify the new makefiles. He is also trying to improve the main makefile based on Mike's suggestions, and considering more circumstances. We will still keep the option that users could use the original makefiles.