DM-13887: Let ap_verify process multiple images per instance #57
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
kfindeisen
force-pushed
the
tickets/DM-13887
branch
from
d938616
to
d28303b
Compare
The original intent was that ap_verify would create a single Job for all metrics computed during execution. This is now neither necessary nor possible, so it's better to leave Job files created by the pipeline untouched.
This change eliminates the need to parse the data ID twice, and will make it easy to switch to parseAndRun later.
kfindeisen
force-pushed
the
tickets/DM-13887
branch
from
d28303b
to
00016e5
Compare
mrawls
approved these changes
Jan 15, 2019
| This argument can be used to run multiple instances of ``ap_verify`` concurrently, with each instance producing output to a different metrics file. | ||
| The template for a file to contain metrics measured by ``ap_verify``, in a format readable by the :doc:`lsst.verify</modules/lsst.verify/index>` framework. | ||
| The string ``{dataId}`` shall be replaced with the data ID associated with the job, and its use is strongly recommended. | ||
| If omitted, the output will go to files named after ``ap_verify.{dataId}.verify.json`` in the user's working directory. |
There was a problem hiding this comment.
Oh... haha... still, I think a putting these in the "output" directory might be a better default.
There was a problem hiding this comment.
Good idea, but I'd rather do this on a new ticket, since it's a change from the pre-existing behavior and I'd want to double-check that custom filenames don't break things (including CI!) in surprising ways.
mrawls
reviewed
Jan 15, 2019
This change involves several coupled decisions, including moving responsibility for Job-level metadata to AutoJob. The main reason this commit can't be broken up is because Job metadata requires a data ID, which is only available during or after pipeline processing. Therefore, removing runApPipe's dependency on Job and moving Job creation inside the data ID loop needs to happen simultaneously. I have not added tests for AutoJob, because the class is being deprecated in favor of MetricsControllerTask.
kfindeisen
force-pushed
the
tickets/DM-13887
branch
from
00016e5
to
eb1924f
Compare
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.
This PR adds multiprocessing support to
ap_verifyby making thepipeline_drivermodule callApPipeTask.parseAndRuninstead ofApPipeTask.runDataRef. This change requires a complete reorganization of howap_verifypasses information fromap_pipeto the metrics code, but as a side benefitap_verifyno longer needs to parse data IDs itself.