Thermodynamics.jl icon indicating copy to clipboard operation
Thermodynamics.jl copied to clipboard

Introduce logger type, remove module methods

Open charleskawczynski opened this issue 1 year ago • 8 comments

This PR removes print_warning and error_on_non_convergence, which allow users to toggle how Thermodynamics does error handling. With this PR, those methods are removed, and instead users pass in objects that dispatch this error handling. One advantage of this is that dispatch will result in statically elided paths. The upsides of this is that:

  1. JET won't complain (although we can currently just overload print_warning and error_on_non_convergence to silence JET)
  2. this PR could result in less register usage

I've looked at NVTX reports for several jobs, and the constructors (where this feature is relevant) are not really a bottleneck. So, 2) may not actually buy us anything.

charleskawczynski avatar Sep 02 '23 17:09 charleskawczynski

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (20de40f) 92.96% compared to head (3d02b13) 92.43%. Report is 7 commits behind head on main.

Files Patch % Lines
src/relations.jl 89.32% 11 Missing :warning:
src/logger.jl 83.33% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
- Coverage   92.96%   92.43%   -0.53%     
==========================================
  Files          10       11       +1     
  Lines        1152     1164      +12     
==========================================
+ Hits         1071     1076       +5     
- Misses         81       88       +7     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 02 '23 17:09 codecov[bot]

Is that something I could also use in CloudMicrophysics to print warnings when someone is trying to use the parameterizations outside of the acceptable range of inputs?

I don't want to throw an error. But it would be great to have a warning of some sort

trontrytel avatar Sep 06 '23 04:09 trontrytel

Is that something I could also use in CloudMicrophysics to print warnings when someone is trying to use the parameterizations outside of the acceptable range of inputs?

Yes

I don't want to throw an error. But it would be great to have a warning of some sort

Generally, this can be used as follows:

if logger isa WarnAndErrorLogger
 # do stuff that is not gpu compatible
end

That way, users can call different style versions:

  • WarnAndErrorLogger perhaps GPU compatible
  • Verbose GPU incompatible, but prints lots of info
  • (maybe) UnsafeLogger GPU compatible, prints no warnings and does not error (by us)

I'm not sure I like the name "logger" though, it's not solely for logging. It's kind of like a log level, maybe that's a better name?

charleskawczynski avatar Sep 06 '23 15:09 charleskawczynski

@vchuravy this is where I wanted some sort of GPU logger.

I'm not 100% sure of names. I would prefer if it didn't get conflated with the existing Julia logging though.

simonbyrne avatar Sep 20 '23 17:09 simonbyrne

It seems like we may no longer have a choice in CUDA 5.0: https://buildkite.com/clima/climaatmos-ci/builds/13625#018b0189-731b-4c9d-970f-554d7c5740b1/145-235. Going to move forward with this. We don't need to actually use this, but it will nevertheless help with upstream inference.

charleskawczynski avatar Oct 05 '23 21:10 charleskawczynski

@simonbyrne, it seems that this functionality will help us get https://github.com/CliMA/ClimaAtmos.jl/pull/2199 merged in. Unless there are naming objections/suggestions, I'd like to merge this.

charleskawczynski avatar Oct 05 '23 21:10 charleskawczynski

Can you please give a brief high-level overview on how this works?

Sbozzolo avatar Feb 09 '24 17:02 Sbozzolo

Yes. Although, I keep having back-and-forth thoughts on this.

One downside that I'm remembering is that: The src code (e.g., in ClimaAtmos) will need to be changed if we leverage this feature to silence JET noise. Right now, we can just use TD. print_warning() = false at the top of the JET script.

Maybe we can put this on hold.

charleskawczynski avatar Feb 09 '24 18:02 charleskawczynski