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

Debug/profile declared private or accessible by CMAKE? #140

Open
cguzman95 opened this issue Apr 7, 2020 · 5 comments
Open

Debug/profile declared private or accessible by CMAKE? #140

cguzman95 opened this issue Apr 7, 2020 · 5 comments
Assignees
Labels

Comments

@cguzman95
Copy link
Collaborator

cguzman95 commented Apr 7, 2020

Hi @mattldawson ,

With all the new components in the GPU side (ODE solver, Linear solver etc) I'm adding a lot of code to debug/print stats(time execution/number of iterations...). I'm setting all of these code under different DEBUG or PRINT flags for each case (maybe I have a DEBUG_ITSOLVER or a DEBUG_CVODE_GPU something like that), instead of mixing all in the DEBUG flag and printing so much info that is hard to read (and also enabling the debug flag increase the total time execution and I want something more independent). So here is my question:

Where should I put this flags? Accessible throught the CMAKE or something private like a #define variable in the specific file that affects (e.g. DEBUG_ITSOLVER where be set in the file itsolver.cu).

In my opinion, it depends on the type of the DEBUG that we are doing:

  • Profiling (time executions and iterations): Set as CMAKE variable. Maybe the user wants to check the performance of a component.
  • Printing variables: Set as private variable in the file. Probably the user doesn't mind about this debug code, but for us developers these variables can help us when making changes.
  • Checkings/testing of individual functions: Set as CMAKE variable?(doubting). Maybe the user wants to check if a specific function in the GPU (e,g. iterative solver) has some difference in accuracy infront their CPU alternative.

Let me know your opinion.

@cguzman95 cguzman95 added this to the GPU chemistry solver milestone Apr 7, 2020
@cguzman95 cguzman95 self-assigned this Apr 7, 2020
@mattldawson
Copy link
Collaborator

I like the idea about providing profiling information to the user if they want it. Eventually, I think we should remove most of the debugging code - once we're satisfied that everything is working, so I think doing a #define in the code is a good choice. But, I think having something like a ENABLE_PROFILIING flag might be useful. Maybe we could replace ENABLE_DEBUG with ENABLE_PROFILING eventually? I think we want to reduce the number of CMake flags because the number of possible combinations of flags is getting too large to make sure they all compile and run.

I agree we want to avoid printing things to the console - particularly when CAMP is running in a 3-D model like MONARCH. I would suggest collecting rather than printing this info when the user requests it.
Currently, when ENABLE_DEBUG is on, the user can a pass a solver_stats_t object to camp_core%run() that collects solver statistics (number of f(), Jac() calls, failure codes, etc.). In MONARCH we plot this during the run to see how the solver is running across all the grid cells. Does it make sense to include GPU profiling in a similar object? or maybe the same one? We could modify this object to include GPU profiling info and be enabled with ENABLE_PROFILING if necessary.

@cguzman95
Copy link
Collaborator Author

cguzman95 commented Apr 8, 2020

About change ENABLE_DEBUG with ENABLE_PROFILING: Maybe yes. After all solver_stats is mainly composed by time executions and number of iterations (e.g LS_setups, Jac_evals_total, Jac_time_s...). Maybe is more "appropiate" call it profiling.

I agree with reducing the number of CMake flags because can be a chaos have all this flags present... But I think we can solve this by dividing the CMake file into multiple CMakes (Like CVODE does). In this way the user will have only one ENABLE_PROFILING flag to enable all, but then when developing we only need to modfiy the specific private CMake file flag variable to print the specific interesting part we want.

In fact, this approach is similar to collecting all the data in solver_stats_t and printing them. The difference is that instead of merge all the stats in the same struct/class, now all the components/classes has his own flag to create, collect and print stats. In this way all the stats are independent and encapsullated and when developing we can easily avoid collecting data from classes than we don't need.

E.g. I only need to print GPU stats, so I left ENABLE_DEBUG OFF and only enabling a flag in the CMake GPU file I can collect this specific data without adding the noise from collecting stats of other parts of the code (This is very important because otherwise a noticeable part of the total time execution comes from collecting stats from all the code, when we only want from a small part).

@mattldawson
Copy link
Collaborator

that sounds good to me - maybe give it a try and we can review and revise as necessary?

@cguzman95
Copy link
Collaborator Author

Yes, I will reallocate my current DEBUG_GPU flag following this idea before the merge with chem_mod

@cguzman95
Copy link
Collaborator Author

Seems CMake definitions only affects the current directory, so I can't apply this definitions in a separate CMakeLists... well, at the moment I left it as a only option ENABLE_DEBUG and put rest of debug definitions alongside add_definitions(-DPMC_USE_GPU) whenENABLE_DEBUG is on

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

No branches or pull requests

2 participants