Update analysis job objects #208
Conversation
|
It's been a flood of pull requests, but this one is also ready for review. |
| @@ -92,6 +95,8 @@ def exists(cls, datatype, command, options): | |||
| The name of the command run on the data | |||
| options : dict | |||
| Options for the command in the format {option: value} | |||
| analysis : Analysis object | |||
| Analysis the job will be added to if it doesn't exist | |||
There was a problem hiding this comment.
This is not clear, could you fix?
|
I also need opinions on one thing: Currently exists is a boolean. This means that if a job exists we can tell it exists, but not which one specifically. This becomes problematic with the create function, as we have no way currently of getting and returning that already existing job. Should create find the existing job and return it, or raise an error like it currently does in the commented out code? And if not, I'm not sure how to handle getting an existing job in this case. |
| sql, (datatype_id, command_id, opts_json)) | ||
| if not analyses: | ||
| return False | ||
| analyses = [x[0] for x in analyses] |
There was a problem hiding this comment.
this, and the following if statement seem odd. Why not just do this in a single list-comp?
There was a problem hiding this comment.
yeah, fixed that but haven't put in the pull since the above discussion.
| analyses.remove(analysis.id) | ||
|
|
||
| # check data used to create jobid | ||
| sql = ("SELECT processed_data_id, array_agg(sample_id ORDER BY " |
There was a problem hiding this comment.
This is probably incorrect, but isn't this query and the subsequent forloop nearly equivalent to:
SELECT 1
FROM (SELECT processed_data_id, array_agg(sample_id ORDER BY sample_id) as samples
FROM qiita.analysis_sample
WHERE analysis_id = %s
GROUP BY processed_data_id)
WHERE samples=foo;There was a problem hiding this comment.
Not really, as the current statement actually creates the dictionary structure in postgres, so the samples are multiple lists tied to their processed data ids. I can't think of a way to do the actual comparison in SQL itself because of that. If we were dealing with a single aggregated list of samples that would work though.
|
I've simplified down the sample equality checking in exists, so it's ready for review again. |
| "GROUP BY processed_data_id") | ||
| samples = dict(conn_handler.execute_fetchall(sql, [analysis.id])) | ||
| # check passed analyses' samples dict against all found analyses | ||
| for aid in analyses: |
There was a problem hiding this comment.
Why not make it a while loop vs a for/break?
There was a problem hiding this comment.
Because I need to loop through all the ids in analyses and it's easier to break that for loop than find a way to do it in a while loop.
There was a problem hiding this comment.
Agree with @squirrelo on this one, the current structure seems fine to me
|
Just that question about the |
|
Oh, I missed your comment from a day ago:
Maybe you can add a parameter like |
|
Yeah, the problem is we then need to search again for the actual job. I suppose this could be alleviated by making exists set up as
and just have returning the job as part of exists, saving a lot of extra lookup time. Combining the two of those would then work to "create" an existing job really easily. |
|
That seems reasonable
|
|
This is ready for more review. |
|
👍 looks good to me |
|
Thanks, merging now. |
This adds some functionality to job objects that is needed for the commands selection page. Exists now properly checks the data used for the job, and delete now works for jobs.