fractal-analytics-platform / fractal-tasks-core

Main tasks for the Fractal analytics platform
https://fractal-analytics-platform.github.io/fractal-tasks-core/
BSD 3-Clause "New" or "Revised" License
14 stars 6 forks source link

Add `overwrite` argument to cellpose task #458

Closed tcompa closed 1 year ago

tcompa commented 1 year ago

This is part of a broader discussion, and not necessarily restricted to cellpose task. Together with issues like #351 or #279 (and to related updates in fractal-server), this issue would contribute to enabling the "I want to play around on a subset of data as part of an experimental workflow branch" use case.

jluethi commented 1 year ago

Could come with an overwrite=True option that is even on by default.

tcompa commented 1 year ago

Default: True

Notes:

  1. overwrite will define the behavior for two cases at the same time:
    • Writing labels
    • Writing tables (in case the output-ROI optional parameter is set)
  2. We should move the check on existing ROIs to the beginning of the task
tcompa commented 1 year ago

As part of #500, we introduced the write_table helper function. In a very similar way, in c6dcae3 I introduced a prepare_label_group helper function, which shares a lot of logic with write_table (e.g. the overwrite-related checks and the setting of attributes for the new label group). The main difference is that prepare_label_group does not handle label writing (while write_table does, through anndata), see explanation in the docstring below.

Screenshot from 2023-08-30 09-22-25

Any feedback is welcome, both on whether we should have this helper function at all and about how it can improve. Note that we could obviously abstract some of the logic shared by write_table and prepare_image_group, but this will make no difference for someone writing a task and therefore does not represent a priority IMO.

tcompa commented 1 year ago

As of https://github.com/fractal-analytics-platform/fractal-tasks-core/commit/d9bc7953df1c4ad355ed27a25766a3b9416b5709, we are now also using the write_table helper function as part of the cellpose task.

tcompa commented 1 year ago

Note: IMO, the two diffs below (from create-ome-zarr and cellpose tasks) clearly supports the choice of introducing write_table

Screenshot from 2023-08-30 09-48-52

Screenshot from 2023-08-30 09-47-42

jluethi commented 1 year ago

Wow, that's a great improvement with write_tables indeed! It's both shorter and, importantly, also much simpler to understand what's happening!

I like the abstraction of the prepare_label_group for those same reasons. Was at first thinking about broadening it more, with also writing the tables. But as you state in the docstring, our region-based writing needs to happen in the task. So I'd say this is the correct trade-off. And looking at the diff for the cellpose task, this abstraction seems very worthwhile!