scitran / core

RESTful API
https://scitran.github.io
MIT License
18 stars 18 forks source link

Use central job enqueue logic for jobs spawned by rules #962

Closed nagem closed 7 years ago

nagem commented 7 years ago

Resolves #923

Changes in this PR

codecov-io commented 7 years ago

Codecov Report

Merging #962 into master will decrease coverage by <.01%. The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #962      +/-   ##
========================================
- Coverage      90%    90%   -0.01%     
========================================
  Files          49     49              
  Lines        6646   6664      +18     
========================================
+ Hits         5982   5998      +16     
- Misses        664    666       +2
ryansanford commented 7 years ago

As-is this change means when job is spawned from gear rule, gears should expect config.json files that contain meta on the inputs, but not contain config.

There are at least 2 gears that don't operate well in this situation (scitran-apps/dcm2niix and flywheel-apps/mriqc) and guessing several more due to the pattern being used which assumes config is included in config.json whenever that file exists.

This situation would no longer be expected once gear rules pass along config defaults from the gear manifest. https://github.com/scitran/core/pull/961

I double checked the gear spec and defaults for config options are not required. @kofalt can you confirm I'm reading the schema correct?

I initially thought it to be good to update those and other gears to support this PR as-is, as those changes would align with the gear spec, and increase their robustness. After recognizing the pattern for how config.json is used, I no longer think merging this before #961 would be prudent.

ryansanford commented 7 years ago

For posterity, here's the project rules I used to make the sequential pipeline of classifier => dcm2niix => (qa-report-fmri and mriqc)

/* 1 */
{
    "_id" : ObjectId("59ee238d35cbd20016b53984"),
    "all" : [ 
        {
            "type" : "file.type",
            "value" : "dicom"
        }
    ],
    "name" : "Run dcm2niix on dicom",
    "alg" : "dcm2niix-rts",
    "project_id" : "59ee238d35cbd20016b53983",
    "any" : [ 
        {
            "type" : "file.measurements",
            "value" : "unknown"
        }, 
        {
            "type" : "file.measurements",
            "value" : "anatomy_inplane"
        }, 
        {
            "type" : "file.measurements",
            "value" : "diffusion_map"
        }, 
        {
            "type" : "file.measurements",
            "value" : "diffusion"
        }, 
        {
            "type" : "file.measurements",
            "value" : "anatomy_t1w"
        }, 
        {
            "type" : "file.measurements",
            "value" : "anatomy_t2w"
        }, 
        {
            "type" : "file.measurements",
            "value" : "anatomy_ir"
        }, 
        {
            "type" : "file.measurements",
            "value" : "functional"
        }, 
        {
            "type" : "file.measurements",
            "value" : "localizer"
        }, 
        {
            "type" : "file.measurements",
            "value" : "field_map"
        }, 
        {
            "type" : "file.measurements",
            "value" : "high_order_shim"
        }, 
        {
            "type" : "file.measurements",
            "value" : "calibration"
        }, 
        {
            "type" : "file.measurements",
            "value" : "functional_map"
        }, 
        {
            "type" : "file.measurements",
            "value" : "coil_survey"
        }, 
        {
            "type" : "file.measurements",
            "value" : "anatomy_pd"
        }, 
        {
            "type" : "file.measurements",
            "value" : "perfusion"
        }, 
        {
            "type" : "file.measurements",
            "value" : "spectroscopy"
        }, 
        {
            "type" : "file.measurements",
            "value" : "phase_map"
        }, 
        {
            "type" : "file.measurements",
            "value" : "screenshot"
        }
    ]
}

/* 2 */
{
    "_id" : ObjectId("59ee238d35cbd20016b53985"),
    "all" : [ 
        {
            "type" : "file.type",
            "value" : "dicom"
        }
    ],
    "name" : "Run dicom-mr-classifier on dicom",
    "alg" : "dicom-mr-classifier",
    "project_id" : "59ee238d35cbd20016b53983",
    "any" : []
}

/* 3 */
{
    "_id" : ObjectId("59ee238d35cbd20016b53986"),
    "all" : [ 
        {
            "type" : "file.type",
            "value" : "nifti"
        }
    ],
    "name" : "Run qa-report-fmri on nifti",
    "alg" : "qa-report-fmri",
    "project_id" : "59ee238d35cbd20016b53983",
    "any" : [ 
        {
            "type" : "file.measurements",
            "value" : "functional"
        }
    ]
}

/* 4 */
{
    "_id" : ObjectId("59ee478ad91dd5001d30ca0a"),
    "all" : [ 
        {
            "type" : "file.type",
            "value" : "nifti"
        }
    ],
    "name" : "mriqc",
    "alg" : "mriqc-rts",
    "project_id" : "59ee238d35cbd20016b53983",
    "any" : [ 
        {
            "type" : "file.measurements",
            "value" : "anatomy_t1w"
        }, 
        {
            "type" : "file.measurements",
            "value" : "anatomy_t2w"
        }, 
        {
            "type" : "file.measurements",
            "value" : "functional"
        }
    ]
}
kofalt commented 7 years ago

No, all default config values are required. It (has generally been) expected that the submitter to jobs-add fill in any default values, for example.

ryansanford commented 7 years ago

LGTM. Nice work!

Confirmed following

nagem commented 7 years ago

Removed unneeded comment after change, will merge when CI is green.