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
11 stars 5 forks source link

Drop overwrite from cellvoyager-to-ome-zarr-compute #722

Closed tcompa closed 2 months ago

tcompa commented 2 months ago

Checklist before merging

github-actions[bot] commented 2 months ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  fractal_tasks_core/tasks
  cellvoyager_to_ome_zarr_compute.py
Project Total  

This report was generated by python-coverage-comment-action

jluethi commented 2 months ago

Looks good to me in general. I think catching the non-overwrite error would not be needed in the task anymore if we set overwrite=True this way.

I wonder whether we should do the same for the MIP task: It doesn't rely on an image already existing, so I see more of a reason to keep the overwrite parameter there for the moment. In Fractal itself, it's always run with the init function which checks whether the plate & well already exist & would overwrite them, but there could actually be a use of this function standalone from its init? I also haven't tested whether the plate overwrite will actually overwrite all the content of the zarr images. I assume so, but haven't tested it.

tcompa commented 2 months ago

I think catching the non-overwrite error would not be needed in the task anymore if we set overwrite=True this way.

Agreed. At first I thought we could keep it anyway, just for safety, but it should be unreachable when running through Fractal. I now removed it.

Note that if that error appears, for whatever unexpected reason, the task still fails (just with a slightly less transparent error message).

jluethi commented 2 months ago

Looks good to me, let's merge this! :)