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

Introduce logger type, remove module methods #158

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Sep 2, 2023

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.

@codecov
Copy link

codecov bot commented Sep 2, 2023

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 ⚠️
src/logger.jl 83.33% 2 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trontrytel
Copy link
Member

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

@charleskawczynski
Copy link
Member Author

charleskawczynski commented Sep 6, 2023

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?

@simonbyrne
Copy link
Member

@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.

@charleskawczynski
Copy link
Member Author

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
Copy link
Member Author

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

@Sbozzolo
Copy link
Member

Sbozzolo commented Feb 9, 2024

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

@charleskawczynski
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Launch Buildkite Add label to launch Buildkite Speculative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants