simplify output of deploy_suite()

ecmwf / pyflow

A high level Python interface to ecFlow allowing the creation of ecFlow suites in a modular and "pythonic" way

https://pyflow-workflow-generator.readthedocs.io/en/latest/

Apache License 2.0

7 stars 7 forks source link

simplify output of deploy_suite() #17

Closed floriankrb closed 1 year ago

FussyDuck commented 1 year ago

All committers have signed the CLA.

corentincarton commented 1 year ago

@floriankrb, we were in the process of revising the deployment process. We usually always deploy in the same target, meaning that (in my case), the overwrite option is always true. Do you have a case where you want this to fail if the files are already present? We should discuss to merge this with https://github.com/ecmwf/pyflow/pull/15

floriankrb commented 1 year ago

Currently, when deploying a suite I have many warnings about files being overwritten. I am ignoring them because I consider that overwriting a file with identical content is ok. In such case, I wouldn't even consider it as overwriting and I would rather not have any message.

But if I was overwriting files with different content, I would like :

either to have a warning.
or to fail (if I want to make sure that I am not breaking other suites).

corentincarton commented 1 year ago

Exactly. I don't think the warning is a good idea as it would be already too late if you care, the files would have been overwritten already. So I suggest the following:

Put overwrite=True as default
If overwrite is False, then throw an error if the file already exists Can you do your PR to the branch feature/git instead? Otherwise we'll conflict.

floriankrb commented 1 year ago

What about this?

overwrite = True : overwrite the file (with a message if file content is different)
overwrite = False, then throw an error if the file already exists and the content of the file would be changed)

In any case, if the content of the file is the same, don't write any message, and don't fail.

As for choosing a default value, this is a separate decision, defaulting to False is safer but more annoying I suppose.

corentincarton commented 1 year ago

Sounds good. It would be interesting as we would then see what files are impacted by the change using overwrite=True. The thing is that I can't find a case where you wouldn't want to overwrite, unless you want to deploy each time in a clean target, but it's not really important to enforce that. That's why I believe it should be set to true by default.

floriankrb commented 1 year ago

Here is the pull request updated rebase on the correct feature branch.

I will let you change the default to overwrite=False it you think it is appropriate.