-
Notifications
You must be signed in to change notification settings - Fork 0
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
DM-46356: Update testing to use new IsrTaskLSST calibration pipeline #41
base: main
Are you sure you want to change the base?
Conversation
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 small pipeline rearranging suggested would be helpful for the future (and to keep all pipeline constructions consistent), but that can be pushed to a separate ticket (since this is already in a mess of merges).
Same with the certify/verify command flips: I think we should do it, but a separate ticket would be fine to make sure the rest of the merge is smooth.
[defects], | ||
[ | ||
getPipeTaskCmd("linearizer", exposureDict["ptcExposurePairs"], "cpLinearizer.yaml"), | ||
getCertifyCmd("linearizer"), |
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.
Should the verify command run before the certify command? I see that the current code matches what's here, but when we run these for real, the verify is run against the uncertified calibration. It probably doesn't matter, but if we have connections that can only be met by certified calibrations, then that could cause problems in production. I'm happy to take any tickets this spawns if something is broken.
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've added DM-46432 to look into this, so it doesn't need to complicate the merging here. That ticket will decide what the right solution is.
DATA/SConscript
Outdated
inputCollections = f"ci_cpp_ptc,{inputCollections}" | ||
|
||
pipelineYaml = os.path.join(PKG_ROOT, "pipelines", pipelineFile) | ||
pipelineYaml = os.path.join(PKG_ROOT, "pipelines", f"LATISS_legacy_{legacyDate}", pipelineFile) |
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 this should be "LATISS", f"legacy_{legacyDate}"
so the ci_cpp_gen3
override pipelines match the paths of the cp_pipe
and cp_verify
legacy pipelines. This will require runIsrLSST.yaml
to be moved into a LATISS
subdirectory, but that will make extensions to different camera datasets easier in the future.
@@ -1,7 +1,7 @@ | |||
description: ci_cpp CROSSTALK calibration construction | |||
instrument: lsst.obs.lsst.Latiss | |||
imports: | |||
- location: $CP_PIPE_DIR/pipelines/LATISS/cpCrosstalk.yaml | |||
- location: $CP_PIPE_DIR/pipelines/LATISS/legacy_202409/cpCrosstalk.yaml |
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.
See comment above about adding subdirectories to have matching pipeline paths.
pipelines/runIsrLSST.yaml
Outdated
class: lsst.ip.isr.IsrTaskLSST | ||
config: | ||
doBrighterFatter: false | ||
doDeferredCharge: false |
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.
Mentioned above for instrument-specific pipeline path.
@@ -1,16 +1,11 @@ | |||
2021052500015: | |||
FAILURES: | |||
- RXX_S00 C11 CR_NOISE | |||
SUCCESS: true |
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.
This is reassuring that we're dealing with defects better now.
65ef716
to
b416b00
Compare
Now setting CI_CPP_LEGACY environment variable to 1 will run the "legacy" pipelines and tests.
b416b00
to
7c4374b
Compare
The BFK itself is still unverified and may not be good; see DM-46445.
The old "legacy" calibration pipelines can still be tested locally by setting the environment variable
export CI_CPP_LEGACY=1
.