Feature: Zarr Conversion - #858
Conversation
Support string and select (dropdown) pipeline parameters in addition to numbers, and add the zarr-conversion pipeline mock data. Render the appropriate control per parameter type, validate string params against an optional regex pattern, and URL-encode file paths before submitting since FSS curls them directly. Restore environment-based load balancer URL selection (removing the temporary staging pin). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| validation: { min: 0 }, | ||
| }, | ||
| ], | ||
| "zarr-conversion": [ |
There was a problem hiding this comment.
mock data to be dropped when discovery is live
There was a problem hiding this comment.
Is there an anticipated date for that?
| selectedKey={ | ||
| optionalSelectorKey ?? undefined | ||
| } | ||
| // null (not undefined) so the dropdown |
There was a problem hiding this comment.
A bit verbose of a comment IMO - the type is defined elsewhere for this variable so perhaps it would belong there
| <BaseComboBox | ||
| label="" | ||
| selectedKey={ | ||
| value !== null && value !== undefined && value !== "" |
There was a problem hiding this comment.
FWIW I feel like using isNil and isEmpty from lodash are handy for making things like this more readable - just an opinion though
| value !== null && value !== undefined && value !== "" | |
| (!isNil(value) && !isEmpty(value)) ? String(value) : null |
| return ( | ||
| <TextField | ||
| type="text" | ||
| value={value !== null && value !== undefined ? String(value) : ""} |
There was a problem hiding this comment.
Same comment here for isNil usage but (again) just an opinion
| className={styles.textField} | ||
| /> | ||
| ); | ||
| case "number": |
There was a problem hiding this comment.
Does this mean the default would also catch the file_paths type?
There was a problem hiding this comment.
No file paths are already filtered out
There was a problem hiding this comment.
Yes, I think an explicit catch of file_paths here would be good to avoid relying on an upstream type guard
| const body: Record<string, unknown> = { files: file_paths }; | ||
| const files = Array.isArray(file_paths) | ||
| ? file_paths.map((p) => PipelineService.encodeFilePath(p)) | ||
| : file_paths; |
There was a problem hiding this comment.
Why do they not need to be encoded when not in an array?
| const files = Array.isArray(file_paths) | ||
| ? file_paths.map((p) => PipelineService.encodeFilePath(p)) | ||
| : file_paths; | ||
| const body: Record<string, unknown> = { files }; |
There was a problem hiding this comment.
Can the type narrow down from unknown? I assume the values that get added below must be of some constrained typing like string | number | boolean etc
There was a problem hiding this comment.
moved away from the unknown typing in 0d86855
| validation: { min: 0 }, | ||
| }, | ||
| ], | ||
| "zarr-conversion": [ |
There was a problem hiding this comment.
Is there an anticipated date for that?
| interface Props { | ||
| className?: string; | ||
| selectedKey?: string; | ||
| selectedKey?: string | null; |
There was a problem hiding this comment.
Should undefined be removed in favor of null?
There was a problem hiding this comment.
null is the clear value so I think having both undefined and null here is ok/
There was a problem hiding this comment.
we also still need undefined because it's currently an optional param, so if it were removed ExistingAnnotationPathway and MetadataDetails would need to be refactored slightly
3 of the 4 parts of it are out for review now |
- Use lodash isNil/isEmpty in PipelineParameterInput (isEmpty only for select case, since it treats numbers as empty) - Narrow encodeFilePath to string -> string and only encode string entries - Trim verbose selectedKey comment in ComputePipelineModal Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aswallace
left a comment
There was a problem hiding this comment.
+1 on Sean's comments, otherwise lgtm
| interface Props { | ||
| className?: string; | ||
| selectedKey?: string; | ||
| selectedKey?: string | null; |
There was a problem hiding this comment.
we also still need undefined because it's currently an optional param, so if it were removed ExistingAnnotationPathway and MetadataDetails would need to be refactored slightly
Form-state values only ever come from text/select inputs, so type them as string. validateParam/onBlur take string | undefined to still flag untouched required fields. Defaults are String()-ified on load. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Hoist filePaths to its own string[] field on ComputeTaskRequest, removing the Array.isArray/typeof guards in submitComputeTask - Type parameters as Record<string, string> (all form values are strings), collapsing the null/undefined guard in the submit loop - Drop redundant `default !== undefined` checks (default is never undefined) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Adds in
zarr-conversionto the pipelines option. In order to accomodate some of the new params, we have to update the pipeline-param display and typing logic.Try it out on https://staging.bff.allencell.org/ select a CZI or ND2, -> process files -> conversion ->submit
🤖 Generated with Claude Code