Skip to content
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

Add -i option in rerun message to ignore existing output directories #326

Closed
wants to merge 1 commit into from

Conversation

tomaslovato
Copy link
Contributor

@tomaslovato tomaslovato commented Oct 17, 2019

In relation to the error described in issue #136
I report here a small change to improve the usability of the tool when re-running diagnostics.

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

Closes #136

@tomaslovato tomaslovato added the bug Something isn't working label Oct 17, 2019
@tomaslovato tomaslovato added enhancement New feature or request and removed bug Something isn't working labels Oct 17, 2019
@valeriupredoi
Copy link
Contributor

valeriupredoi commented Oct 21, 2019

related to ESMValGroup/ESMValTool#1271

The implementation from ESMValTool/esmvaltool/diag_scripts/shared/_base.py is up to the user's choice so I'd add a line pointing the user to the documentation and tell them it's up to them to use it (there are repercussions!) 🍺

@tomaslovato
Copy link
Contributor Author

@valeriupredoi So far the only repercussion I see is the overwriting of previous results, but it is a clear consequence of the fact that a user wants to re-run the diagnostic. This should simply make more easy to copy and paste the commands to re-run the code ... am I missing something else?

@valeriupredoi
Copy link
Contributor

valeriupredoi commented Nov 13, 2019

@tomaslovato yep that's it! But what I suggested was to add a line in the stdout message to tell the user that stuff will be overwritten and point them to the documentation ie line 432: logger.info("To re-run this diagnostic script, run:\n%s", rerun_msg) add the fact that -i option is used by default 🍺

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current re-run message is a good default, because it's safe. I agree that it would be nice to mention the other options, but I would prefer a log message instead of changing the default. Maybe something like:

if self.script.lower().endswith('.py'):
    logger.info("Add the flag --help to the command for re-running the diagnostic script to see the available options for overwriting/ignoring previous results.")

@valeriupredoi
Copy link
Contributor

closed via ESMValGroup/ESMValTool#1317

@valeriupredoi valeriupredoi deleted the development_rerun_diagnostic branch February 20, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re-run diagnostic script stuck
3 participants