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

verdi plugin list: Show which exit codes invalidate cache #5710

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Oct 21, 2022

Fixes #5707

For Process entry points, the verdi plugin list command will show a
formatted rendering of the process specification. Here we mark exit
codes that invalidate the cache by printing in bold red. For example,
the ArithmeticAddCalculation will now display as:

$ verdi plugin list aiida.calculations core.arithmetic.add
...
Exit codes:
Exit codes that invalidate the cache are marked in bold red

  1  The process has failed with an unspecified error.
  2  The process failed with legacy failure mode.
 11  The process did not register a required output.
100  The process did not have the required `retrieved` output.
110  The job ran out of memory.
120  The job ran out of walltime.
310  The output file could not be read.
320  The output file contains invalid output.
410  The sum of the operands is a negative number.

@sphuber sphuber requested a review from ltalirz October 21, 2022 14:53
@sphuber
Copy link
Contributor Author

sphuber commented Oct 21, 2022

@ltalirz this one builds on top of #5709 which should be reviewed and merged first.

No longer blocked because #5709 has been merged.

@sphuber sphuber force-pushed the feature/5707/plugin-list-exit-code-invalidates-cache branch from cc4f2a7 to 6cbb7b1 Compare October 21, 2022 15:05
@sphuber sphuber force-pushed the feature/5707/plugin-list-exit-code-invalidates-cache branch 2 times, most recently from d47b1d9 to 71446ad Compare October 21, 2022 17:58
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Great, thanks @sphuber !

Funny, I was preparing an alternative layout to your original one with the [x], and it was pretty much exactly the changes you made by yourself (color, remove the colon) ;-)
The only further change I would suggest is to move the explanation of the colors to the bottom, below the list.

P.S. While thinking about how to present this list, I started wondering whether our default (valid cache = true) is the right choice.

As a plugin developer, when everything goes well, you don't need to return an exit code and it will pick 0 by default. But when something goes wrong, I think by default you want the result to be invalid for the cache, unless you explicitly say that "something went wrong but the result is still usable".

Anyhow, it's a bit late to change that now.

P.P.S 0 is not part of the list but is certainly a valid exit code.

@sphuber sphuber force-pushed the feature/5707/plugin-list-exit-code-invalidates-cache branch from 71446ad to 6eac855 Compare October 22, 2022 12:21
@sphuber
Copy link
Contributor Author

sphuber commented Oct 22, 2022

P.S. While thinking about how to present this list, I started wondering whether our default (valid cache = true) is the right choice.

Yeah, you may be right here.

Anyhow, it's a bit late to change that now.

I'm afraid so 😅. You could open an issue and we could try to think if we can provide a deprecation pathway to change the default by v3.0. It might be possible to do this.

The only further change I would suggest is to move the explanation of the colors to the bottom, below the list.

Done!

The only thing I noticed when doing this (actually when writing the commit message) is that this way of styling (i.e. without explicit symbol) will be problematic for terminals that don't support bold or colored output. There the user won't see any difference. Since we were already relying on bold output to have particular meaning (for inputs and outputs being required or not), I decided that we can keep it for now. But this could be a problem at some point. Not sure how common it is for terminals to not support bold colored text.

For `Process` entry points, the `verdi plugin list` command will show a
formatted rendering of the process specification. Here we mark exit
codes that invalidate the cache in bold red. For example, the
`ArithmeticAddCalculation` will now display as:

    $ verdi plugin list aiida.calculations core.arithmetic.add
    ...
    Exit codes:

      0  The process finished successfully.
      1  The process has failed with an unspecified error.
      2  The process failed with legacy failure mode.
     11  The process did not register a required output.
    100  The process did not have the required `retrieved` output.
    110  The job ran out of memory.
    120  The job ran out of walltime.
    310  The output file could not be read.
    320  The output file contains invalid output.
    410  The sum of the operands is a negative number.

    Exit codes that invalidate the cache are marked in bold red.
@sphuber sphuber force-pushed the feature/5707/plugin-list-exit-code-invalidates-cache branch from 6eac855 to 3cffd22 Compare October 22, 2022 12:23
@sphuber sphuber requested a review from ltalirz October 22, 2022 12:24
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks for this addition @sphuber !

very useful

@sphuber sphuber merged commit 59f5914 into aiidateam:main Oct 24, 2022
@sphuber sphuber deleted the feature/5707/plugin-list-exit-code-invalidates-cache branch October 24, 2022 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

verdi plugin list aiida.calculations: Show which exit codes invalidate the cache
2 participants