DiamondLightSource / httomo

High-throughput tomography pipeline
https://diamondlightsource.github.io/httomo/
BSD 3-Clause "New" or "Revised" License
4 stars 3 forks source link

Expose `asynchronous` image saver parameter as `cluster` parameter in httomo pipelines #260

Closed yousefmoazzam closed 2 months ago

yousefmoazzam commented 3 months ago

One suggestion where this could happen is perhaps somewhere in the image saver method wrapper, it could change the cluster parameter name in the dict of parameters to be asynchronous.

Possibly somewhere around the following code, where the offset parameter is being added to the dict of parameters: https://github.com/DiamondLightSource/httomo/blob/4b87e12ce8e689c229df58946c3424c036dcf3d0/httomo/method_wrappers/images.py#L53-L63

yousefmoazzam commented 2 months ago

Has this actually been completed? On docs_update, it still looks like the template exposes the asynchronous parameter to users rather than exposing it as a cluster parameter, judging from how the template generator script is: https://github.com/DiamondLightSource/httomo/blob/55733f82add9f1c56e46495067211ba241111cdb/yaml_templates/yaml_templates_generator.py#L116-L117

and also how the YAML template exposes asynchronous rather than cluster: https://github.com/DiamondLightSource/httomo/blob/55733f82add9f1c56e46495067211ba241111cdb/yaml_templates/httomolib/2.0/httomolib.misc.images/save_to_images.yaml#L12

(The asynchronous parameter was indeed exposed to users, which was done in the referenced PR, but this issue is talking about potentially exposing the asynchronous parameter as cluster instead - to make it easier to understand for httomo users to decide between sync/async versions of the image saver).

dkazanc commented 2 months ago

I think we can leave it (as not doing it) for now as for larger images it won't bring a significant gain with asynchronous write. I think we probably need to test it first to see exactly when it might be needed to users. Does it make sense?

yousefmoazzam commented 2 months ago

Yeah that's fine, I'll switch it to "Closed as not planned" then, since it's not been "completed"