JP-4129: Work around default CRDS log configuration to allow stpipe capture#312
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
==========================================
+ Coverage 95.66% 95.71% +0.05%
==========================================
Files 35 35
Lines 3391 3433 +42
==========================================
+ Hits 3244 3286 +42
Misses 147 147 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@braingram @pllim - I remembered why we couldn't directly capture the CRDS logger in the initial logging refactoring. It's because CRDS configures a logger on import, including adding a non-optional stream handler. If stpipe attaches its own handlers without removing the existing stream handler, users will see duplicate log messages with slightly different formats. It occurs to me looking at it now, though, that since we have special handling for "py.warnings", there's no reason we couldn't also have special handling for the CRDS logger. The stpipe and crds packages already have a special relationship, so we don't need to treat it like any other anonymous dependency. This PR is a proposal that uses the CRDS log interface to remove its own stream handler when logging is configured (via stpipe call or command line) and restore it when the logging configuration is undone. With this change, and adding "CRDS" to the stpipe loggers for jwst, I see CRDS logging messages captured in the datamodel logs and no duplicate messages in the terminal. If I run a If we like this proposal I can add some tests for it. Edit: to clarify, this does not fix the timing problem for JP-4129, where the initial messages from pipeline start up are missed in the datamodel logs, it just fixes the separate log capture and configuration problem for the CRDS logger specifically. |
pllim
left a comment
There was a problem hiding this comment.
I like the idea in principle! I will let stpipe maintainers do the actual code review. Thanks!
|
Thanks! Looks great. How would you like to coordinate this update with the other logging work? I doubt the code changes will overlap but the tests might (and this might be easier to test in combination with the other logging changes). |
|
I can wait for this one to go in first. |
|
Sounds good, I'll clean it up and add some tests. The initial CRDS messages aren't the only ones issued while running most steps, so I think it should still be easy enough to test this without the other log changes. |
|
Regtests with jwst and romancal on main: Tests with CRDS added to the stpipe loggers for jwst (spacetelescope/jwst#10523): All passing. |
|
To test locally on one of the regression test exposures, along with jwst PR spacetelescope/jwst#10523: Then, there should be a CRDS message in the cal logs for the output model: prints |
braingram
left a comment
There was a problem hiding this comment.
Thanks for putting this together. I left a few suggestions about underscoring the new crds_client functions (since I'm assuming we don't want this to be misinterpreted as public api).
I'd also be ok with in-lining the calls to crds.core.log (since the new functions only have 1 line of code).
Can (and should) we check if the crds console handler is already detached/attached so the stpipe logging configuration doesn't apply or remove something the user configured?
Will do.
I put them in the crds_client module because there are warnings there about not importing crds directly anywhere but in that module.
The remove function won't do anything if there's no handler attached currently, so that should be okay as is. I was on the fence about checking whether there was previously a console logger before adding it back in the undo. I can look into it now. |
Thanks for pointing that out. It looks like it was introduced in: |
|
Okay, I removed the functions from crds_client and called the CRDS log interface directly from stpipe.log instead. Also now checking for a previous console before adding one in for |
braingram
left a comment
There was a problem hiding this comment.
Testing locally works a treat!
Thanks for making this improvement.
Toward JP-4129
Workaround for CRDS logging configuration, to allow stpipe to capture log messages from the "CRDS" logger.
The default console logger assigned by CRDS is removed when stpipe configures loggers, then is restored when the configuration is undone.
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)stpipe@git+https://github.com/<fork>/stpipe.git@<branch>)jwstregression testromancalregression test