To review: Optional arrays/objects should not have side effects

[tcompa]

EDIT: The most compelling specific issue is now part of fractal-analytics-platform/fractal-tasks-core#448. This issue is now to track the broader discussion on how to proceed regarding:

1. scope setting: which use cases will be supported in fractal-web? Where will this information be documented?
2. fractal_tasks_core development: will there be an automated check of scope compliance
3. custom task packages: the requirements from point 1 should be made extremely clear

---

(coming from discussions with @rkpasia)

Consider this task

def task_function(x: Optional[list[str]] = None) -> str:
    if x is None:
        return "A"
    elif len(x) == 0:
        return "B"
    else:
        return "C"

which has a different behavior for x=None and x=[]. I think this is a bad practice, and should be avoided.

The reason for this issue comes from fractal-analytics-platform/fractal-web#205. Depending on how we proceed in fractal-web, it's not clear whether setting x=[] will be allowed. In general, it's better to stay on the safe side and make sure that tasks are robust.

[tcompa]

Note: this issue is not yet well defined, as it largely depends on how things evolve in fractal-web. It's also possible that it won't be relevant for 0.10.0, if everything appears to work "as is", and we'll tackle it later.

(also: I edited the example above, for clarity)

[tcompa]

A relevant example is currently in copy_ome_zarr:

• If ROI_table_names is None, we use some defaults.
• If it is an empty list, we use the empty list.

We can use this as a good (and very simple) example for further discussions/tests.

[tcompa]

Related to this, and to fractal-analytics-platform/fractal-web#218:

• Is there any task where we typically pass an argument/attribute like [] or {} (as first-level argument or nested attribute of other arguments) and obtain a specific behavior that differs from the case where that argument/attribute is unset?

If there is no such case (or if it is not used often), then we can proceed as in fractal-analytics-platform/fractal-web#218. Otherwise we need to review this issue.

[tcompa]

Tasks I'm reviewing (check mark means I did not identify any scenario in which [] or {} is a typical/useful input).

• cellpose_segmentation.py
• copy_ome_zarr.py
• create_ome_zarr_multiplex.py
• create_ome_zarr.py
• illumination_correction.py
• maximum_intensity_projection.py
• napari_workflows_wrapper.py
• yokogawa_to_ome_zarr.py
• lib_input_models.py
• lib_channels.py

[tcompa]

Re: copy_ome_zarr.py
This is an at-risk task, due to #6. It would become an issue if someone wants to use different ROI tables than the default ones.

Re: create_ome_zarr_multiplex.py:

• image_glob_patterns could be [] or None
• if image_glob_patterns: treats both in the same way
• same in read_mlf_file: line if filename_patterns: treats both in the same way
• if metadata_table_files: does the same as above
• if metadata_table_files is None: would lead to different behaviors, but there is no scenario where providing metadata_table_files={} is meaningful.

Re: create_ome_zarr.py

• image_glob_patterns are handled as in create_ome_zarr_multiplex
• allowed_channels=[] is not a meaningful argument

Re: napari_workflows_wrapper

• input_specs={} in principle may exist, if we set up a napari workflow which has no input (maybe a workflow that uses some internal testing data?). It's clearly not a typical use case, so it's OK that we currently won't expose it through fractal-web
• output_specs={} doesn't make sense

Summary:

• copy_ome_zarr is the only real issue -> I will open another issue for this, since I think we can fix it.
• Checking this by hand is obviously not a good way to go. We should move towards narrowing the scope of what is properly supported in fractal-web and then (1) state it clearly in the docs, (2) add automatic tests wherever possible.

[tcompa]

Addendum:

Re: lib_input_models.py

• In NapariWorkflowsInput, setting channel={} is not a meaningful use case

Re: lib_channels.py

• In OmeroChannel, setting window={} is not a meaningful use case

[tcompa]

cc @lorenzocerrone