-
Notifications
You must be signed in to change notification settings - Fork 7
Update tasks to Fractal 2.0 #671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
An interesting observation: Using the new zarr_url scheme, we can drop Path as an import from many of these tasks => promising for the direction of having cloud-compatible tasks! :) |
Should we rename the type "3D" to "is_3D" or something similar? To avoid having to switch to different dictionary approach |
Note: I pushed 34faa92, with these changes:
|
While reviewing the In the future this task would benefit from yet another review. |
Thanks a lot for the review & the fixes @tcompa ! |
Closes #669
Closes #682
Closes #324
Closes #415
Closes #299
Closes #523
Closes #535
Closes #686
Closes #674
Closes #691
Discussion
To what degree are we giving deprecation warnings or just changing some of the fractal-tasks-core functions?
In general, public functions should stay stable. But we probably have some functions that shouldn't be public functions. For example,
utils.get_table_path_dict
: I'm now updating it to follow the new behavior.But also: zarr_url vs. zarrurl (e.g. in pyramid building) and other functions.
I would expect this to become the 1.0 release of fractal-tasks-core. We have some other libraries depending on it, but not too many yet. Going forward, we need to take care not to break any public functions. This may be a good time to make some of those functions private now though.
To review
Using zarr_url instead of path (see Decide on name for zarr path / location identifier & unify use across the repo #670)
Task output for apply_registration_to_image:
Edited images:
An image zarr_url is provided that is already in the image => start from previous properties, gets updated with the manifest-based new filters (=> new types)
Should manifest types be applied to input zarr_urls by default even if no image_list_update output is provided?
Task output in compound tasks: Can tasks provide custom Pydantic models as output? e.g. for the Cellvoyager converter: Can it produce
InitArgsCellVoyager
objects as output or just dicts that can be cast to that model in the input of the compute task?We can use the models to validate the dict we're building in the init task, but still output a dict in the init task
Potential complexity argues for handling it in the task (=> more control to task developer).
=> We stay with dict output for init tasks (in init_args)
General recommendation: Create a dict, optionally validate it with a model
=> That scenario doesn't matter, it was just init_args. If compute task output contains None => unsetting would make sense. A task should be able to unset an attribute. Unsetting via providing "my_attr": None
Currently is an error.
Handling on the task side: Avoid putting in None when
(exclude_Nones, exclude_unset)
=> How do we handle the complexities in InitArgs?
In general, try to keep InitArgs simple
In general, the less hidden behaviors the better! We need to document any hidden behaviors or ways of doing the same thing in 2 ways.
Cleanup
fractal_tasks_core.utils.get_parameters_from_metadata
be deprecated?create_ome_zarr
&yokogawa_to_ome_zarr
functionsChecklist before merging
CHANGELOG.md