Add required flag to DIVE_PARAM - #1639
Conversation
Superseded by the `required` flag on DIVE_PARAM (see PR #1639). VIAME \`.pipe\` files using the old `# Requirements:` header should be migrated to the new spec — until they are, those parameters won't be prompted for. - Drop \`PipelineRequirement\` from apispec.ts and dive_utils/types.py. - Drop \`description\` and \`requirements\` fields from \`Pipe\` / \`PipelineDescription\`; description is already exposed via \`metadata.description\`. - Drop \`extract_pipe_description\`, \`extract_pipe_requirements\`, \`extractPipeDescription\`, \`extractPipeRequirements\`. Training config descriptions now read from \`extractPipeMetadata().description\`. - Drop the Requirements dialog from RunPipelineMenu.
389b487 to
a10f41e
Compare
|
Hi, For example to pass from: And we would be able to combine it with the required argument: It is a bit more verbose, but I think it could be more scalable, add avoid problems if later we have a string parameter for a type. What do you think of this? |
06e04b6 to
0feff06
Compare
| const rules: Record<string, ValidationRule> = { | ||
| integer: (value: string | number) => Number.isInteger(Number(value)) || 'Please enter a whole number', | ||
| positive: (value: string | number) => Number(value) >= 0 || 'Please enter a number ≥ 0', | ||
| strictlyPositive: (value: string | number) => Number(value) > 0 || 'Please enter a number > 0', | ||
| }; | ||
|
|
||
| function getRules(type: PipelineParamType): ValidationRule[] { | ||
| const res = []; | ||
| function getRules(type: PipelineParamType, required = false): ValidationRule[] { | ||
| const res: ValidationRule[] = []; | ||
| if (required) { | ||
| res.push((value) => { | ||
| if (value === undefined || value === null || value === '') return 'Required'; | ||
| return true; | ||
| }); | ||
| } | ||
| if (type.includes('int')) { | ||
| res.push(rules.integer); | ||
| } |
There was a problem hiding this comment.
I think we can integrate the rule directly in the rules dict to have it consistent.
| } | |
| const rules: Record<string, ValidationRule> = { | |
| integer: (value: string | number) => Number.isInteger(Number(value)) || 'Please enter a whole number', | |
| positive: (value: string | number) => Number(value) >= 0 || 'Please enter a number ≥ 0', | |
| strictlyPositive: (value: string | number) => Number(value) > 0 || 'Please enter a number > 0', | |
| required: (value: string | number) => (value !== undefined && value !== null && value !== '') || 'This field cannot be empty', | |
| }; | |
| function getRules(type: PipelineParamType, required = false): ValidationRule[] { | |
| const res: ValidationRule[] = []; | |
| if (required) { | |
| res.push(rules.required); | |
| } | |
| if (type.includes('int')) { | |
| res.push(rules.integer); | |
| } |
There was a problem hiding this comment.
Otherwise looks good to me
Pipelines can now mark a DIVE_PARAM as required by adding `required`
as one of the type props in the comment header:
# DIVE_PARAM ["My label", float, required]
:my:key = 0.5
The parser strips it from `type_props` and sets `required: true` on
the param. PipelineParamsDialog shows a red `*` on the label and
blocks the Run button via vuetify form validation if the field is
empty. Both the desktop common.ts parser and the server
pipeline_discovery.py parser handle the keyword.
…he gear icon or when a param is required
d851158 to
5f3ebb5
Compare
Probably a good idea if we plan on adding a lot more parameters, though my text editor is starting to get sad because the max line ending on these pipes and that would further extend it. I'm kind of hoping we can partially do away with this system at some point because of that (e.g. taking the kwiver type definitions and adding max ranges and things to those macros so they don't need to be re specified). So I'm fine if it goes in as it is now because I want to refactor this eventually anyway. Another thing I wouldn't mind doing is just adding a quick comment up here next to the parameters like # DP1, #DP2 for short, then define the actual parameters for DIVE elsewhere in the file, like references in a latex paper |
Plain `mdi-cog-outline` gear for pipelines with DIVE params (still its own click target for the params dialog) and no icon for pipelines without, as before the params-dialog rework.
sprokit's pipe parser rejects a one-line `config <key> = <value>` statement, so kwiver globals must stay in config blocks. Track `config <key>` block context in both discovery parsers so `config global` + `:scale ... # DIVE_PARAM [...]` yields key `global:scale` instead of bare `scale`.
Wrapper pipes that only `include` another pipe now inherit its params
instead of having to redeclare them. A file's own declarations override
inherited ones for the same key, matching kwiver's config override
order. Unresolvable includes ($ENV{...} paths handled by kwiver's own
search path) contribute no params.
BryonLewis
left a comment
There was a problem hiding this comment.
Going to merge for proper testing of the pipeline params withe newer sealion metadata pipelines
Summary
Pipelines can now mark a `DIVE_PARAM` as required so the user can't run the pipeline until they supply a value, and `config ` lines (kwiver globals) can be DIVE_PARAM targets — same syntax, parser-side recognition only.
Spec
```
:my:key = 0.5 # DIVE_PARAM ["My label", float, required]
config global:fps = 5 # DIVE_PARAM ["FPS", int, required]
```
`required` is a flag keyword in the DIVE_PARAM comma list — stripped from `type_props`, sets `required: true` on the param.
Coverage
Changes