Add options to cancel jobs, redo existing large images, and not force jobs #1586
Conversation
| .param('createJobs', 'Whether job(s) should be used to create each ' | ||
| 'large image. Select true to use a job if needed, false to ' | ||
| 'never use a job, and always to always use a job.', | ||
| required=False, default='false', dataType='string', | ||
| enum=['true', 'false', 'always']) |
There was a problem hiding this comment.
Based on @manthey 's description in #1139 I get the impression this should be split into two options.
ie 'force' for always and 'localJob' making both boolean as seen here:
large_image/girder/girder_large_image/rest/tiles.py
Lines 199 to 205 in 56193ce
There was a problem hiding this comment.
The endpoint's previous iteration had separate 'force' and 'localJobs' parameters, then createJobs would be set to either True or 'always' internally, as seen below. We still might not want to create jobs at all, so @manthey suggested the endpoint's parameter accept a third state. The string to boolean conversion is not my favorite, though.
| default=False, dataType='boolean') | ||
| .param('recurse', 'Whether child folders should be recursed.', required=False, | ||
| default=False, dataType='boolean') | ||
| .param('recurse', 'Whether child folders should be recursed.', |
There was a problem hiding this comment.
I generally prefer definitions not to include the option unless very self-explanatory. Perhaps something like:
'If true, images in child folders will also be checked'
Also as a personal preference I would change the descriptions from 'Whether...' to 'If...'
There was a problem hiding this comment.
That reads clearer and explaining what happens depending on what the user selects is a good idea. Change made here.
| recurse=params.get('recurse'), createJobs=createJobs, | ||
| localJobs=params.get('localJobs'), | ||
| cancelJobs=params.get('cancelJobs'), | ||
| redo=params.get('redoExisting')) |
There was a problem hiding this comment.
I think breaking the options apart as suggested above would allow you to simplify this.
|
|
||
| def createLargeImagesRecurse( | ||
| self, folder, user, recurse, createJobs, localJobs, cancelJobs, redo): | ||
| result = {'childFoldersRecursed': 0, | ||
| 'itemsSkipped': 0, | ||
| 'jobsCanceled': 0, | ||
| 'jobsFailedToCancel': 0, | ||
| 'largeImagesCreated': 0, | ||
| 'largeImagesRemovedAndRecreated': 0, | ||
| 'totalItems': 0} | ||
| if recurse: | ||
| for childFolder in Folder().childFolders(parent=folder, parentType='folder'): | ||
| result['childFoldersRecursed'] += 1 | ||
| childResult = self.createImagesRecurseOption(folder=childFolder, | ||
| createJobs=createJobs, user=user, | ||
| recurse=recurse, localJobs=localJobs) | ||
| childResult = self.createLargeImagesRecurse(folder=childFolder, user=user, | ||
| recurse=recurse, createJobs=createJobs, | ||
| localJobs=localJobs, | ||
| cancelJobs=cancelJobs, redo=redo) |
There was a problem hiding this comment.
This feels logically backwards to me. I think I would expect:
def createLargeImage(self, folder):
for item in folder:
***createImage logic here***
def folderRecurse(self, params):
if recurse:
for childFolder in folder:
createLargeImage()
additionally I am not sure I would make a new function to check the recurse param. That is a design choice for you and David to decide though.
gabrielle6249
force-pushed
the
cancel-pending-jobs
branch
from
6ce57a0
to
e22df94
Compare
This adds two more options from #1139 to the PUT large_image/folder/{id}/tiles endpoint, and allows the createJobs filter to accept three states. The first flag allows cancelling unfinished large image jobs related to items in the folder, so the large image can then be removed and a new one created. The second gives the option to remove and redo existing large images in the folder. The three states createJobs now accepts can either never use a job, use one only if needed, or force one for every large image.