-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
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. |
f98b3b6
to
e344e11
Compare
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 |
Yes
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:
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? |
5db75c5
to
47ad077
Compare
@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. |
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. |
@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. |
47ad077
to
0835ee1
Compare
0835ee1
to
3d02b13
Compare
Can you please give a brief high-level overview on how this works? |
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 Maybe we can put this on hold. |
This PR removes
print_warning
anderror_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:print_warning
anderror_on_non_convergence
to silence JET)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.