JP-4129: Capture pre-pipeline CRDS checks in ASDF log (try 2)#309
JP-4129: Capture pre-pipeline CRDS checks in ASDF log (try 2)#309pllim wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
==========================================
+ Coverage 95.71% 95.73% +0.01%
==========================================
Files 35 35
Lines 3433 3447 +14
==========================================
+ Hits 3286 3300 +14
Misses 147 147 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for Capture pre-pipeline CRDS checks in ASDF log (try 2) spacetelescope/stpipe#309
|
@braingram does this approach still give you the duplicates you reported in #304 (comment) ? |
|
Would you post some examples of log differences with this PR for some representative jwst and romancal pipeline and step runs? |
|
I can give a jwst example. My env is not set up to run roman stuff. |
pllim
force-pushed
the
log-all-the-things-porque-no-los-dos
branch
from
23526ae to
221f113
Compare
|
For JWST, I tested using https://github.com/spacetelescope/jwst-pipeline-notebooks/tree/main/notebooks/MIRI/Imaging notebook that David Law mentioned from a previous logging ticket I worked on (JP-4128). I focused on the Stage 3 processing, as follows, though you can run whatever steps you wish to test: if doimage3:
i2d_result = Image3Pipeline.call(
association_im3,
output_dir=image3_dir,
steps=image3dict,
save_results=True
)
else:
print('Skipping Spec3 processing')In the notebook, when that cell is run, you can immediately see these logs in the cell output. This part of the behavior is the same with and without this patch: After that is done, now I have a for line in i2d_f.cal_logs.calwebb_image3:
print(line)With this patch, the log messages that JP-4129 said were missing now show up: Without this patch (e.g., using |
|
Thanks. I'd like to see what this does for romancal runs before approving. Also, would you add some tests for this change in behavior? |
|
Testing with romancal and running one of the L1 regtest files I see the following in the log output: However the cal_logs is missing messages prior to Would you rerun your test this time using |
|
Re: strun -- Good catch. Looks like I have to also patch here: Line 469 in 1459190 Also, question: Why doesn't the strun interface use |
|
Follow-up on strun problem: Turns out to be more complicated than I thought. I need the Line 457 in 1459190 To avoid breaking existing behavior, I guess I can call that line twice, once to just get the |
and also add a change log first to reserve PR num [ci skip]
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
pllim
force-pushed
the
log-all-the-things-porque-no-los-dos
branch
from
221f113 to
9a34957
Compare
[ci skip]
| log_names=step_class.get_stpipe_loggers(), | ||
| formatter=step_class._log_records_formatter, | ||
| ) as log_records: | ||
| step, step_class, positional, debug_on_exception = ( |
There was a problem hiding this comment.
The only non-destructive way I can think of right now is calling just_the_step_from_cmdline twice but this does not seem good enough because:
- It is calling the same function twice
- It causes original log to screen to repeat some stuff twice but finally it captures stuff only once in the datamodel log
- It still misses the CRDS logs because the CRDS logs already emitted in the first call
There was a problem hiding this comment.
I am still stuck here:
Lines 282 to 285 in 14c9627
I cannot figure out an easy way to get the step_class before load_config_file. I need the former to build the log context manager, but the latter emits log messages.
There was a problem hiding this comment.
Or maybe we can just give up on fixing this for strun and tell the user to call it the Pythonic way if they really care about capturing those few lines of log messages in datamodels.
There was a problem hiding this comment.
The lines in cmdline.py in the above comment are no longer needed following the removal of step_script. I took a stab at cleaning some of it up in #317 which allowed attaching the recording handler earlier in the process. The whole submodule is quite convoluted so maybe we want to do even more cleanup (or split #317 up). I'm open to suggestions.
There was a problem hiding this comment.
I don't know why the CLI is designed this way. As an outsider, I am curious why CLI doesn't just use Step.call or something from the Step class directly. Why all this extra processing that is only available via CLI?
There was a problem hiding this comment.
Some of it is unavoidably linked to the CLI/strun if we want to support mapping the spec to the argparser to get things like useful --help messages. strun also supports a few things that call does not including:
- running from a config/asdf file
- saving parameters (although this is a bit weird since it then tries to run the step which I wouldn't expect)
- "debug" mode
Using Step.call could simplify things and seems worth looking into. One complication is that we'll still have to handle merging config and CLI arguments (since they can be used together when both are provided). We can't just defer to Step.from_config_file/section (since that only uses the config file). We might be able to use the config_file argument to call but I'm not sure if it's equivalent. It's very confusing that there so many ways to do purportedly the same thing.
There was a problem hiding this comment.
running from a config/asdf file
I think this is the source of my problem. It reads the config first in order to even find step class. But I need the step class for logging. So when it runs this way, by the time I get the step class to set up logging, the config logs are already gone. I don't think I can win here.
It's very confusing that there so many ways to do purportedly the same thing.
Exactly!
|
Update: I am guessing that this PR will have a lot of conflicts after #317 so I am prepared to close this without merge and then rethink my approach, if there is even a problem left to be solved then. This PR is in a "wait and see" mode. Just want to let you know I am aware of the situation. Thanks for your help! |
|
Superseded by #317 thanks to Brett Graham. Melanie Clarke will close out the JIRA ticket as complete. No more work is needed, so closing this without merge. Thanks, all! |
3 participants
Resolves JP-4129
Blocked by:
TODO:
strunTasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)stpipe@git+https://github.com/pllim/stpipe.git@log-all-the-things-porque-no-los-dos)jwstregression testromancalregression test