Repository navigation
adding Resource component. - #252
Conversation
… schema to determine whether to display text fields and check boxes associated with the resources used by the workflow
yousefmoazzam
left a comment
yousefmoazzam
left a comment
There was a problem hiding this comment.
Cool, I can see that it indeed displays the resource parameter form for the ptypy and pytrex param config components, thanks.
Just a few bits to tidy up in the comments below.
There was a problem hiding this comment.
Firstly, is there a particular reason that you are developing on a fork and not a branch off of this repo?
Secondly, I made the comments below but then I had another idea:
- make a resource parameter type {memory: string | null; nprocs: number | null; usegpu: boolean | null}
- turn the dataMap function into a getDefaults function and parse the arguments and fill in the argument default (you will likely need to assert the type) or leave it as null
- do what I said about a ternary operator in the resource parameter states, checking if the resource item exists in the resource object returned from getDefaults and set accordingly.
- when rendering the TextFields, you can just do resourceObject.memory && ...
| function dataMap(obj: any) { | ||
| return Object.keys(obj).reduce(function (result: any, key: any) { | ||
| if (key === "memory" || key === "nprocs" || key === "usegpu") { | ||
| result[key] = obj[key]; |
There was a problem hiding this comment.
This will still throw a linting error
| if (Object.hasOwn(resourceDisplayList, "memory")) { | ||
| defaultMem = resourceDisplayList.memory.default; | ||
| } | ||
| if (Object.hasOwn(resourceDisplayList, "nprocs")) { | ||
| defaultNprocs = resourceDisplayList.nprocs.default; | ||
| } | ||
| if (Object.hasOwn(resourceDisplayList, "usegpu")) { | ||
| defaultGpu = resourceDisplayList.usegpu.default; | ||
| } |
There was a problem hiding this comment.
Rather than checking if the object has these keys all the time, it might be worth storing them as a variable and only checking once. It will certainly look nicer.
… attempt at addressing the types, more concise logic around what components to display
yousefmoazzam
left a comment
yousefmoazzam
left a comment
There was a problem hiding this comment.
Thanks for taking the time to address the suggestions in the reviews, I appreciate there was a lot thrown at you.
I can see there are potentially two minor comments leftover, but I think these can be pursued in the future, I think this is more than good enough for now.
The majority of the suggested changes have been implemented, the finer details can be addressed in the future.
which looks at the arguments of a workflow schema to determine whether to display text fields and check boxes associated with the resources used by the workflow. It also does not cause an error if used on a template without any resource defined within the schema. see the example below:
There are aspect which could be improved such as the typing and filtering of the query data in ParameterConfiguration.
also the setting of defaults within ResourceForm could also be improved I suspect. Futhermore, I updated the types name for Tomo workflows to Httomo such that they are displayed in imaginghub. also within types I changed I13-2 into I13 and set there default workflow to Httomo as I think this might more in line with what they do day to day (but feel free to change this back if I am incorrect, or just need to be a different pull request). Any feedback is welcome.