Add more cases for std stream logging #3347
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, stdout/err logs not output if the streams were not set (i.e. defaulting to None) and logged with the entire stream target if set. This PR adds more cases here with case specific logging: if stdout/err is not redirected, that is now logged. If tuple mode is used, the tuple is unpacked and logged as separate fields. If an unknown format is specified an error is logged (but no exception is raised - it is not the job of this code to validate the stdout/err specifications) This PR is in preparation for a new case to be added in an upcoming PR which requires more serious formatting that was not accomodated by the previous implementation.
khk-globus
reviewed
Apr 11, 2024
Comment on lines
+1403
to
+1404
| else: | ||
| logger.error(f"{name} for task {tid} has unknown specification: {target!r}") |
There was a problem hiding this comment.
If this is an error, should it also raise an exception? Otherwise, it feels more like a warning, no?
There was a problem hiding this comment.
It's an error. It should be validated elsewhere and this code path should be unreachable (and so do something more like raise an assertion that this path is "impossible".
"should" in the sense of thats how things could be but aren't. If you get this message, there's a problem and your workflow won't run as you expect. But this log statement is not the place to be validating that user input: prior to this, this code would log at info level whatever garbage you had fed in, and it will continue to log that garbage here in this case.
khk-globus
approved these changes
Apr 11, 2024
benclifford
added a commit
that referenced
this pull request
Apr 20, 2024
PR #3347 attempted to do case-based logging of all the different kinds of stdout/err specification. It failed to capture some of the cases involving os.PathLike, and so after PR #3347, those cases would log an ERROR that the specification was unknown. This new behavior is only a new ERROR logging message - PR #3347 did not change other behaviour. This PR also amends a rich test of stdout/err specification types introduced in PR #3363 to check that no ERROR messages are logged during these tests.
benclifford
added a commit
that referenced
this pull request
Apr 22, 2024
PR #3347 attempted to do case-based logging of all the different kinds of stdout/err specification. It failed to capture some of the cases involving os.PathLike, and so after PR #3347, those cases would log an ERROR that the specification was unknown. This new behavior is only a new ERROR logging message - PR #3347 did not change other behaviour. This PR also amends a rich test of stdout/err specification types introduced in PR #3363 to check that no ERROR messages are logged during these tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, stdout/err logs no output if the streams were not set (i.e. defaulting to None) and logged with the entire stream target if set.
This PR adds more cases here with case specific logging: if stdout/err is not redirected, that is now logged. If tuple mode is used, the tuple is unpacked and logged as separate fields. If an unknown format is specified an error is logged (but no exception is raised - it is not the job of this code to validate the stdout/err specifications)
This PR is in preparation for a new case to be added in an upcoming PR which requires more serious formatting that was not accomodated by the previous implementation.
Changed Behaviour
log messages - including a new error log
Type of change