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-39122: Remove UnresolvedRefWarning filters #236
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
+ Coverage 84.41% 84.48% +0.06%
==========================================
Files 50 50
Lines 4621 4557 -64
Branches 826 817 -9
==========================================
- Hits 3901 3850 -51
+ Misses 531 517 -14
- Partials 189 190 +1
☔ View full report in Codecov by Sentry. |
Additionally PreExecInit is cleaned up to always expect resolved refs in quantum graph and use those refs to store initOutputs. `CmdLineFwk` now checks that output runs on command line and in graph are consistent.
f3d8804
to
8d21bf0
Compare
@TallJimbo, could you check my last commit 6e7444c (replaces 18faa70) which removes skip-existing-in from SingleQuantumExecutor (and replaces it with extend_run option). I re-started Jenkins to see if it breaks anything. |
18faa70
to
6e7444c
Compare
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.
Looks good to me, and I think it's important that we get the behavior here consistent with the pipetask
CLI docs (as this does). But the Prompt Processing Framework also uses some of these lower-level Python interfaces (via SeparablePipelineExecutor)
now, so I'd also like to hear from @kfindeisen on whether there are any changes here that would be problematic, particularly:
- There's a new mandatory
extend_run
argument toSeparablePipelineExecutor
construction. --clobber-outputs
without--skip-existing
is no longer possible.
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 pointing me to this PR, @TallJimbo! I do have some questions about the implications for Prompt Processing, since (for performance reasons) we use runs in a very different pattern from batch processing.
6e7444c
to
59ace30
Compare
SingleQuantumExecutor is updated to use resolved references and `Datastore.mexist()` method when searching for existing quantum outputs. The search is now done only in output run instead of all collections defined in `skip_existing_in` (GraphBuilder will skip quanta with existing outputs from that list). SingleQuantumExecutor will clobber full existing outputs if `clobber_outputs` is set and `skip_existing_in` does not include output run. `CmdLineFwk` always adds output run to `skip_existing_in` if `extend_run` is set, but `SeparablePipelineExecutor` has more open configuration.
59ace30
to
7e0685b
Compare
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.
Looks good. Sorry I helped send you off on a wild-goose chase in the middle of this.
Additionally PreExecInit is cleaned up to always expect resolved refs in quantum graph and use those refs to store initOutputs.
CmdLineFwk
now checks that output runs on command line and in graph are consistent.Checklist
doc/changes