lightning-flash icon indicating copy to clipboard operation
lightning-flash copied to clipboard

Flash Zero: Allow to customize `LightningCLI`

Open daavoo opened this issue 3 years ago • 4 comments

🚀 Feature

Expose options to customize the instantiation of LightningCLI in Flash Zero commands.

Motivation

I would like to be able to customize the instantiation. For example, to not use SaveConfigCallback which is hardcoded in:

https://github.com/Lightning-AI/lightning-flash/blob/0253d71588b582da326bb01ef116ebd7cd61f21e/flash/core/utilities/flash_cli.py#L166

Pitch

flash --save-config-callback False {task}

Alternatives

Do not use Flash Zero directly and instantiate LightningCLI manually.

daavoo avatar Aug 31 '22 16:08 daavoo

Hi, @daavoo - Thank you for creating the issue! I think it makes sense, note that LightningCLI has save_config_callback and we can set it to None in LightningCLI but not in Flash Zero.

Myabe, we can think on the name, as having a boolean flag for save-config-callback might confuse users with save_config_callback in LightningCLI. Let's use, --save-config? cc: @ethanwharris - What do you think?

Just wondering, if you would like to take this up and create a PR? If you think you can help setup a draft PR, I can take it from there. :) Otherwise, I can help add this flag.

krshrimali avatar Sep 01 '22 07:09 krshrimali

Hi, @daavoo - Thank you for creating the issue! I think it makes sense, note that LightningCLI has save_config_callback and we can set it to None in LightningCLI but not in Flash Zero.

Yes, in the example I set it to False but it has the same effect as None because the check is if self.save_config_callback.

Myabe, we can think on the name, as having a boolean flag for save-config-callback might confuse users with save_config_callback in LightningCLI. Let's use, --save-config? cc: @ethanwharris - What do you think?

No strong opinion on name.

Just wondering, if you would like to take this up and create a PR? If you think you can help setup a draft PR, I can take it from there. :) Otherwise, I can help add this flag.

Sure thing. I wanted to open the issue first to check if it made sense for you

daavoo avatar Sep 01 '22 08:09 daavoo

I think a PR to add a boolean flag --save-config to enable/disable save config callback will be a good idea! Regarding the name, if there are any other suggestions, we can rename it quickly in the PR before merging. :)

I'll be looking forward to your PR, thank you very much!

krshrimali avatar Sep 01 '22 09:09 krshrimali

@daavoo are you interested in looking at it?

Borda avatar Dec 05 '22 03:12 Borda