adding User.jobs() #2035
adding User.jobs() #2035
Conversation
|
Changes Unknown when pulling 83d0b2b on antgonza:user-jobs into ** on biocore:master**. |
|
Changes Unknown when pulling b6ab4ed on antgonza:user-jobs into ** on biocore:master**. |
|
Turns out that you can remove processingjobs via the processingworkflow method, which is much cleaner than adding a processingjob remove. However, the processing job didn't have a method to return its processingworkflow. Just added it. |
|
Changes Unknown when pulling 6b3c252 on antgonza:user-jobs into ** on biocore:master**. |
| qdb.sql_connection.TRN.add(sql, [self.id]) | ||
| r = qdb.sql_connection.TRN.execute_fetchindex() | ||
|
|
||
| return (None if not r |
There was a problem hiding this comment.
what about return qdb.processing_job.ProcessingWorkflow(r[0][0]) if r else None? I don't think the use of a negative conditional is necessary
| cmds = {} | ||
| for j in user.jobs(): | ||
| cmd = j.command | ||
| if cmd not in cmds: |
There was a problem hiding this comment.
is there a valid situation in which the same command will appear multiple times?
There was a problem hiding this comment.
yes, multiple and the idea of cache; basically, the command is the definition of the command (split libs, demux, etc) and the job is the command with the specific params.
There was a problem hiding this comment.
I don't see how this caches anything though? cmds is reset prior to the forloop so the only compute that is avoided is the assignment of a value to a key in a dict?
There was a problem hiding this comment.
...on closer inspection, it doesn't look like cmds is needed at all
| cmd = j.command | ||
| if cmd not in cmds: | ||
| cmds[cmd] = cmd | ||
| ccmd = cmds[cmd] |
There was a problem hiding this comment.
ccmd is cmd -> True, is there a need for this additional variable?
There was a problem hiding this comment.
Not necessary but I like it cause that makes the subsequent calls smaller. However, we are only retrieving the name of the command so change it a little bit.
| class UserJobs(BaseHandler): | ||
| @authenticated | ||
| def get(self): | ||
| response = user_jobs_get_req(self.current_user) |
There was a problem hiding this comment.
this executes as a transaction and has the potential to block as it is performing IO. Should this method be a tornado coroutine instead?
| cmds = {} | ||
| for j in user.jobs(): | ||
| cmd = j.command | ||
| if cmd not in cmds: |
There was a problem hiding this comment.
yes, multiple and the idea of cache; basically, the command is the definition of the command (split libs, demux, etc) and the job is the command with the specific params.
| cmd = j.command | ||
| if cmd not in cmds: | ||
| cmds[cmd] = cmd | ||
| ccmd = cmds[cmd] |
There was a problem hiding this comment.
Not necessary but I like it cause that makes the subsequent calls smaller. However, we are only retrieving the name of the command so change it a little bit.
| class UserJobs(BaseHandler): | ||
| @authenticated | ||
| def get(self): | ||
| response = user_jobs_get_req(self.current_user) |
|
Changes Unknown when pulling c2582e9 on antgonza:user-jobs into ** on biocore:master**. |
| @authenticated | ||
| @coroutine | ||
| def get(self): | ||
| response = user_jobs_get_req(self.current_user) |
There was a problem hiding this comment.
I believe this needs take the result of a yield?
| @@ -1,8 +1,10 @@ | |||
| from tornado.web import authenticated, HTTPError | |||
| from tornado.gen import coroutine | |||
There was a problem hiding this comment.
There are flake8 errors because this import is not being used.
|
Changes Unknown when pulling 88367bf on antgonza:user-jobs into ** on biocore:master**. |
This retrieves the jobs for a user. Note that this only retrieves the processing jobs, AKA no moi jobs. However, AFAIK only the analysis is currently using moi jobs and @josenavas is working on removing that as part of the refactor. I'll do another PR to add the GUI part of this functionality. This should be reviewed and merged before the other PR.