Analysis endpoints update

[ambrussimon]

Resolves #1058

Breaking changes

Create analysis with job

Old - fails schema validation

POST /api/<cont_name>/<cont_id>/analyses?job=true

{"analysis": {"label": "with-job"},
 "job": {"gear_id": gearid,
         "inputs": {"csv": {"type": "acquisition", "id": acqid, "name": "file.csv"}}}

New - no URL param, analysis payload contains job

POST /api/<cont_name>/<cont_id>/analyses

{"label": "with-job",
 "job": {"gear_id": gearid,
         "inputs": {"csv": {"type": "acquisition", "id": acqid, "name": "file.csv"}}}

Create analysis without job

Old - accepted as legacy method

POST /api/<cont_name>/<cont_id>/analyses

// File form with both input and output files, plus metadata below
{"label": "no-job",
 "inputs": [{"name": "input.txt"}],
 "outputs": [{"name": "output.txt"}]}

New - JSON payload instead of file form, inputs only, inputs are filerefs

POST /api/<cont_name>/<cont_id>/analyses

{"label": "no-job",
 "inputs": [{"type": "acquisition", "id": acqid, "name": "input.txt"}]}

Get analysis

GET /api/<cont_name>/<cont_id>/analyses/<analysis_id>
GET /api/analyses/<analysis_id>

Old response - inputs and outputs under files, tagged

{ ...
 "files": [{"name": "input.txt", ... , "input": true},
           {"name": "output.txt", ... , "output": true}],
... }

New response - inputs under inputs, no input/output tag

{ ...
 "inputs": [{"name": "input.txt", ... }],
 "files": [{"name": "output.txt", ... }],
... }

Download analysis file(s)

GET /api/<cont_name>/<cont_id>/analyses/<analysis_id>/files[/filename]
GET /api/analyses/<analysis_id>/files[/filename]

Old response - for inputs and outputs

New response - for outputs only

Additive changes

Download analysis input(s) - works exactly like outputs (/files)

GET /api/<cont_name>/<cont_id>/analyses/<analysis_id>/inputs[/filename]
GET /api/analyses/<analysis_id>/inputs[/filename]

Note that creating an inputs only download ticket via /downloads is NOT supported.

Upload analysis outputs - only allowed when it's without a job

POST /api/<cont_name>/<cont_id>/analyses/<analysis_id>/files
POST /api/analyses/<analysis_id>/files

// File form with output files, plus metadata below
[{"name": "output1.txt", ... },
 {"name": "output2.txt", ... }]

Review Checklist

• Tests were added to cover all code changes
• Documentation was added / updated
• Code and tests follow standards in CONTRIBUTING.md

[codecov-io]

Codecov Report

Merging #1079 into master will decrease coverage by 0.05%. The diff coverage is 97.95%.

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
- Coverage   90.96%   90.91%   -0.06%     
==========================================
  Files          49       49              
  Lines        7041     7055      +14     
==========================================
+ Hits         6405     6414       +9     
- Misses        636      641       +5

[ehlertjd]

This is a breaking change for scitran/client and scitran/python-client.

[nagem]

@lmperry mind taking a look at the changes described above and let us know if anything isn't how you would expect it?

[nagem]

LGTM other than the comment I mentioned above. Let's hold off on a merge until @lmperry gets a chance to take a look, though.

[lmperry]

@nagem, thanks for pinging me on this. LGTM.

[nagem]

@ambrussimon and I talked about this offline but we'll also need a db update to bring existing analysis into the new format (separate input and output arrays).

[nagem]

@ambrussimon I made a small change to be kind to requests that don't specify the correct filegroup type when trying to download an individual file. Makes it less of a breaking change for the frontend client.

[kofalt]

Overall this looks like a positive change, nice work @ambrussimon. Quick questions:

1. I don't see a database migration, so the DB format is staying the same, correct? If so, does the change to gears.py still make sense?
2. Is it worth fixing TargetedPlacer to take multiple files, rather than splitting off a fixed version into TargetedMultiPlacer?

[nagem]

I don't see a database migration, so the DB format is staying the same, correct? If so, does the change to gears.py still make sense?

The DB migration was referenced in a comment above, it's currently under construction.

[kofalt]

3. I notice from the SDK PR that we're using a user string on AdhocAnalysis. Any chance that could just be a normal origin struct?
4. Sanity check, it's now entirely up to the analysis creator if a job should be created, right? There's no field you have to set to prevent that from happening?

[kofalt]

@nagem solid, let's double-check that change after the upgrade. I'd be a fan of outputs on files which wouldn't require any further change, but anything works :)

[nagem]

3

Nice catch, that should be set by core based on request origin, not specified by the user. I think that was old logic from the original legacy style that ended up sticking around.

4

Correct

[ambrussimon]

2

Schema-wise that's non-reconcilable (cannot make endpoints accept {one} OR [{more}, {files}]), or at least not easily/I failed at it. I also imagine it's non-trivial to handle such a schema in the SDK. I'm already sort of pushing the limits of what can/should be handled in a single endpoint with analyses.

Instead, I'd suggest a breaking change in the future, where TargetedPlacer := TargetedMultiPlacer, eg. we use a list of files even if length == 1. Unless anyone's aware of clients or something heavily relying on that not breaking.

[nagem]

Upgrade looks good 👍