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

Pre-evaluation and discussion on branches to be merged in the master #144

Open
andreamarini opened this issue Oct 26, 2024 · 12 comments
Open

Comments

@andreamarini
Copy link
Member

tech-master and tech-maintenance will be merged in the new release in less then a montb.

I opened this issue to start discussing and addressing the specific aspects (non general comments here) of the branches to be merged.

I start with two questions:

  • why did you rename src/modules?
  • why did you rename memory.h

I will post here more questions as soon as I analyze the source.

@sangallidavide
Copy link
Member

The reason is that memory.h and libmodules.a are very general and present in the os.
So we wanted to be sure to avoid linking the wrong file.
On my machine for example it exist
/usr/include/memory.h

  • memory.h --> y_memory.h renaming was done. But I think it never gave problems.
  • libmodules.a --> libYmodules.a I think was done because it gave some problems at a certain point.
    @nicspalla might have more info

Anyway, I agree that we should carefully checked this before doing the changes.

As a side remark. It is a bad practice to have all modules in a folder, it would be better to have the modulse spread across the different folders, each one associated with the connected libxxx.a

@andreamarini
Copy link
Member Author

The module library is present in Yambo from the very beginning (more than 20 years) and never gave a problem.

Moreover also in my machines there exists a memory.h. Actually there are many.

The problem is not in the existence of the file but in the path given at the linking trough -I.

So unless there are general arguments I would prefer to avoid these kind of global renaming.

@attacc
Copy link
Contributor

attacc commented Oct 28, 2024 via email

@andrea-ferretti
Copy link
Member

I think the need to modify Modules -> YModules (as well memory.h in y_memory.h) came from compilation issues found on LUMI using the cray compiler and OpenMP-GPU. I think at the time we resorted to this as the only way to workaround the problem.
I see two options:

  • we revert the change and find a workaround for compilation (without guarantee it would be trivial)
  • we take this as an opportunity to make the code more robust against linking (as Claudio suggested).
  • Ideally we did the same also for kind.h -> y_kind.h, that eventually I reverted since it was not blocking but involved ydriver (which I didn't want to mess up with)

@muralidhar-nalabothula
Copy link
Contributor

#include <memory.h> has an implementation-defined behavior, meaning its behavior may vary depending on the compiler. For instance, gcc or clang might search the user-provided include path first and then the system headers, while the Cray C compiler might prioritize the system headers before the user-provided directories.

If you prefer to stick with the previous file names, here are some alternatives :

  1. #include "../modules/memory.h": This method uses a relative path to the header, eliminating the need to specify linker options.
  2. #include "modules/memory.h" or #include <modules/memory.h>: In this case, provide the include path using -I/path/to/yambo/src.

Each approach has its advantages and disadvantages.

@sangallidavide
Copy link
Member

sangallidavide commented Oct 28, 2024

Thanks Murali.

Options such as #include "../modules/memory.h" would require anyway to change all code lines. So I'd go for #include <y_memory.h>

Since we are touching this part.
The file y_memory / memory.h contains these lines at the top

 use y_memory,     ONLY:MEM_err,MEM_msg,MEM_count,MEM_count_d,MEM_global_mesg,IPL
#if defined _OPENACC || defined _OPENMP_GPU
 use devxlib,      ONLY:devxlib_map,devxlib_unmap,devxlib_mapped,devxlib_memcpy_h2d
#endif
 implicit none

which forces its placement in the source.

I'd rather remove them
a) define a macro USE_MEMORY
b) put implicit none back into the source

Doing so the header can go in his correct place (e.g. as header of the subroutine), and (eventually) does not need to be repeated in the same file

#if defined _OPENACC || defined _OPENMP_GPU
define USE_MEMORY \
 use y_memory,     ONLY:MEM_err,MEM_msg,MEM_count,MEM_count_d,MEM_global_mesg,IPL NEWLINE \
 use devxlib,      ONLY:devxlib_map,devxlib_unmap,devxlib_mapped,devxlib_memcpy_h2d
#else
define USE_MEMORY \
 use y_memory,     ONLY:MEM_err,MEM_msg,MEM_count,MEM_count_d,MEM_global_mesg,IPL
#ednif

Not a fan of the NEWLINE thing that I invented ... but I do not have a better solution for multi-line macros.

@andreamarini
Copy link
Member Author

Thanks Murali.

I personally like Murali's proposal.

Options such as #include "../modules/memory.h" would require anyway to change all code lines. So I'd go for #include <y_memory.h>

Same lines that are touched by changing memory.h into y_memory.h

@andreamarini
Copy link
Member Author

I think the need to modify Modules -> YModules (as well memory.h in y_memory.h) came from compilation issues found on LUMI using the cray compiler and OpenMP-GPU. I think at the time we resorted to this as the only way to workaround the problem. I see two options:

  • we revert the change and find a workaround for compilation (without guarantee it would be trivial)
  • we take this as an opportunity to make the code more robust against linking (as Claudio suggested).
  • Ideally we did the same also for kind.h -> y_kind.h, that eventually I reverted since it was not blocking but involved ydriver (which I didn't want to mess up with)

Instead of changing the code it would be enough to append Y to all libraries at the linking time as it is done with the Ydriver libraries that depend on the executable.

@andreamarini
Copy link
Member Author

As decided previously, only robot-green branches can enter the release.

Note that tech-master is not robot green

https://media.yambo-code.eu/robots/GPL/tech-master/mo.farah.2.php

I am currently working on maintenance-master (synced with phys-master)

@sangallidavide
Copy link
Member

Please notice that the configure for CUDA changed in tech-master to account for other options (e.g. OpenAcc / OpenMP-gpu).

For cuda fortran:
OLD syntax: --enable-cuda="cuda12.6,cc75"
NEW syntax: --enable-cuda-fortran --with-cuda-runtime=12.6 --with-cuda-cc=75

We should add a robot for OpenAcc as well ...

@andreamarini
Copy link
Member Author

For cuda fortran: OLD syntax: --enable-cuda="cuda12.6,cc75" NEW syntax: --enable-cuda-fortran --with-cuda-runtime=12.6 --with-cuda-cc=75

It is already there after a discussion with Nicola 2 weeks ago. Check

https://github.com/yambo-code/yambo-testing-robots/blob/master/mo.farah/marini/CONFIGURATIONS/default.sh

I am not an expert of CUDA so, please CUDA experts, fix the errors (there are many)

andreamarini added a commit that referenced this issue Oct 29, 2024
MODIFIED *  config/mk/global/actions/compile_interfaces.mk config/mk/global/actions/compile_yambo.mk config/mk/global/actions/compile_ypp.mk config/mk/global/functions/mk_exe.mk configure include/version/version.m4 sbin/compilation/helper.sh sbin/compilation/stamp_remove.sh sbin/compilation/verbosity.sh

Additions:
- Now yambo and ypp append to the libraries a _YPP_/_Y_. This solves the problem reported in #144

Patch sent by:  Andrea Marini <[email protected]>
@andreamarini
Copy link
Member Author

A part of Issue #146 the compilation with libraries tagged using _YPP_ and _Y_ has been implented in maintain-compilation.

No need to rename folders and the changes are just few and very localized.

So I propose to revert the renaming of src/modules.

I will try @muralidhar-nalabothula suggestion also in the same branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants