Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Consolidate cmor setups and logging #261
Consolidate cmor setups and logging #261
Changes from 17 commits
fdbe225
580a1a4
0a380d4
20feb79
1aea912
811d208
14c50ef
c2bf739
fe151b3
cd381e8
f8aeadd
1094638
76490c2
c087201
93238fd
d670c1f
3019e37
d268ca5
db96ce2
7e83c4f
7a1c302
2b12405
94b4e70
6fd1670
6e4bec0
28f82c5
5e60221
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this line anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my question: You would need to have found the logfile, in order to read the comment that the log output was being written to the file you are reading. Does that make sense? Is the idea that the logfile may later be moved, and you want a record of where it was when it was being written (full-path)? Otherwise, it seems oddly self-referential.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining. That makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you have more update than what is described in the PR. Can you list these changes too?
For example, here you update the logic to capture
num_failure
to exite3sm_to_cmip
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, "failed" was coming up False, and the function returning 0, when there were indeed failures. So I augmented the condition at the bottom to ensure a non-zero return when any failure was encountered.
This is questionable. When running multiple variables in parallel, should a single variable failure be treated as a failure (non-zero return) or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the behavior should be the same as with a serial run. Maybe what it should do is continue running even with failures, capture the failures, then output the list of failures at the end. That way
e3sm_to_cmip
doesn't end prematurely and other variables that might be successful can be CMORized still. This would make debugging more precise because of e2c ends prematurely, other variables might also fail but we won't know until later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I'll see if I can engineer this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TonyB9000 Can you open a separate GitHub issue for my comment above? If we decide to address it, then it should be performed in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line breaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions
black
and use type annotations.set_log_level
tolog_level
and passlogging
levels (e.g.,logging.level
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird - the "suggested change" (above) looks like no change, due to wrapping."
Addressed 2 and 3, and performed black.
Are type annotations really needed? There are scored of function definitions in mpas.py, etc, and none of them employ type annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agreed to remove this logic and make
set_log_level
thelogging
module levels instead of string?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also suggest renaming
set_log_level
tolog_level
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier - each of the handler modules would need to "import logging", just to be able to pass the defined values "logging.WARNING", "logging.INFO", etc. I can do that. - I don't suppose any significant overhead is incurred in having each handler "import" another module ("logging", on top of our "_logger.py"). It would certainly make "_logger" cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read somewhere that by setting handlers = [], one can avoid "accumulating" handlers in the logger. Global logging is still a bit mysterious.