Improve deprecated messages from `verdi setup` and `verdi code setup`

[agoscinski]

Goal

The two main problems I wanted to solve was to add a deprecation warning to the help page of verdi setup and verdi code setup (easy) as well as printing the deprecation warning before the user has to go through all the prompts (hard). The solution became quite complicated, so I made a second solution that does not do this and just accepts that it is printed afterwards. There are two commits, one for each solution.

Question

Right now my question is if the complexity introduced by making a new class VerdiCommand is worth to have the warning before the prompts. I am not sure. If we choose the second solution (warning comes after prompts), I would change some things (bring back the deprecated_command decorater as it gives us more flexibility).

Technical aspects about the solution printing the warning before the prompt

Setting the deprecated flag from click.command is not enough, since the click.options are parsed (and the prompts appear) before the command is invoked where the logic for printing the deprecated warning lies. Therefore I used VerdiCommandGroup.get_command to print something before. This has the the effect that the deprecation warning is also printed when one requests the help page. One can argue if this is desired behavior.

In principle I thought I could just use click.command's deprecated flag to consume it (set it to false after a print) in VerdiCommandGroup.get_command, but then one gets problems with the validate_consistency test in the verdi-autodocs hook that does not know about this behavior and complains because the help page is not correct (deprecated flag changes also documentation). One could add also this logic to the validate_consistency.py but this adds another point where we need to know about the internal behavior of the cmdline that diverges from click, I was already not a big fan adding one.

[agoscinski]

I removed the second solution in the second commit, because I think that makes reviewing more cumbersome. I think one can imagine how it looks like only keeping the deprecate in the help pages

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.87%. Comparing base (ef60b66) to head (048211c). Report is 33 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #6433 +/- ## ========================================== + Coverage 77.51% 77.87% +0.37% ========================================== Files 560 562 +2 Lines 41444 41802 +358 ========================================== + Hits 32120 32549 +429 + Misses 9324 9253 -71 ``` | [Flag](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6433/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | Coverage Δ | | |---|---|---| | [presto](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | `73.20% <100.00%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[sphuber]

@agoscinski do you have the time to finish this? Happy to take over from here if not

[agoscinski]

Thanks @agoscinski . I am not sure I quite understand the use of the VerdiCommand class. Why don't you simply use the deprecated keyword of the click.Command class? It would have made sense if you would have changed the type for verdi_deprecated to str but you declare it as a bool just as the one defined by click.

I was using first the deprecated flag of click.command, but my added logic and the logic inside click.Command clashed a bit meaning that we got two deprecated messages (One added by the logic in this PR that comes before the prompts and one after the prompts that comes from the logic in click.Command). In your suggested solution this would also happen as a nonempty string would evaluated to True inside click.Command. I tried to set the flag to false after the first error message was printed, but this causes problems with the help page that is inconsistent between runtime and validation.

Reflecting on all solutions, accepting the fact that there are two deprecation message is worth the simplicity of the implementation (your suggestion) in my opinion.

[agoscinski]

Just to give an example how it looks like

(here as copyable text)

(aiida-dev) alexgo@fw:~/code/aiida-core(verdi-improve-deprecate-message)$ verdi code setup --label addnew --computer localhost --remote-abs-path /home/alexgo/code/aiida-core/remoteabs -n
Deprecated: This command is deprecated. Please refer to help page for new usage.
DeprecationWarning: The command 'setup' is deprecated.
/home/alexgo/code/aiida-core/src/aiida/orm/utils/builders/code.py:18: AiidaDeprecationWarning: This module is deprecated. To create a new code instance, simply use the constructor. (this will be removed in v3)
  warn_deprecation('This module is deprecated. To create a new code instance, simply use the constructor.', version=3)
Success: Code<22> addnew@localhost created

[agoscinski]

With prompts it looks like this

[sphuber]

Reflecting on all solutions, accepting the fact that there are two deprecation message is worth the simplicity of the implementation (your suggestion) in my opinion.

Yes, I think having two instead of 1 that comes too late is a much better situation. Another benefit is that the docstrings are automatically updated and we don't risk forgetting it. So please go far the suggested solution

[agoscinski]

When implementing this solution I realized that VerdiCommandGroup.get_command is also used when a list of subcommands is retrieved for the help page which then causes the deprecated warning to kick in when verdi code is run for example (because the subcommand setup is retrieved) . This is very confusing.

This is because of this part in the code

 cmd_name, cmd, args = self.resolve_command(ctx, args) 
 assert cmd is not None 
 ctx.invoked_subcommand = cmd_name 
 super().invoke(ctx) 
 sub_ctx = cmd.make_context(cmd_name, args, parent=ctx) 
 with sub_ctx: 
     return _process_result(sub_ctx.command.invoke(sub_ctx)) 

https://github.com/pallets/click/blob/ac56c4551fcc5d4a60a66b252d6666bf58248e03/src/click/core.py#L1678-L1684 Within resolve_command the get_command is used just to check if the command is available. Within make_context the get_command is used to get the subcommands parameters (and if needed activates the prompts with parse_args somewhere down there).

I did not find any attribute that gives me a hint in which context the get_command is invoked, so my only solution was to directly overwrite the parse_args class that invokes these prompts and print the deprecated message there. https://github.com/pallets/click/blob/ac56c4551fcc5d4a60a66b252d6666bf58248e03/src/click/core.py#L1395 This again requires our own VerdiCommand class.

[sphuber]

I did not find any attribute that gives me a hint in which context the get_command is invoked, so my only solution was to directly overwrite the parse_args class that invokes these prompts and print the deprecated message there. https://github.com/pallets/click/blob/ac56c4551fcc5d4a60a66b252d6666bf58248e03/src/click/core.py#L1395 This again requires our own VerdiCommand class.

There might be another solution. The formatting when you call verdi code --help is done in MultiCommand.format_commands. The Group is a subclass of MultiCommand. So we could override the format_commands in our VerdiCommandGroup:

    def format_commands(self, ctx: Context, formatter: HelpFormatter) -> None:
        """Extra format methods for multi methods that adds all the commands
        after the options.
        """
        from gettext import gettext as _

        commands = []
        for subcommand in self.list_commands(ctx):
            cmd = self.get_command(ctx, subcommand, print_deprecation=False)
            # What is this, the tool lied about a command.  Ignore it
            if cmd is None:
                continue
            if cmd.hidden:
                continue

            commands.append((subcommand, cmd))

        # allow for 3 times the default spacing
        if len(commands):
            limit = formatter.width - 6 - max(len(cmd[0]) for cmd in commands)

            rows = []
            for subcommand, cmd in commands:
                help = cmd.get_short_help_str(limit)
                rows.append((subcommand, help))

            if rows:
                with formatter.section(_("Commands")):
                    formatter.write_dl(rows)

Unfortunately this is quite a bit of code. Only thing I had to change is add the import and then add print_deprecation=False in the self.get_command call.

You then update the signature of that method as:

    def get_command(self, ctx: click.Context, cmd_name: str, print_deprecation: bool = True) -> click.Command | None:

And in the implementation you do:

        if cmd is not None:
            if cmd.deprecated and not ctx.resilient_parsing and print_deprecation:
                from aiida.cmdline.utils.echo import echo_deprecated
                echo_deprecated(cmd.deprecated if isinstance(cmd.deprecated, str) else 'This command is deprecated.')

            return self.add_verbosity_option(cmd)

From some quick testing this seems to work as expected and desired. But I may have missed another edge case. Please give that a try though. Although not ideal that we have to completely copy the format_commands implementation, it might still be preferable than introducing our own command subclass.

[agoscinski]

I feel like both solutions are equally not great in terms of adding extra logic that requires some deeper understanding how click works. My only argument favoring my suggestion is that we can deprecate commands that use other groups like LazyConfigureGroup and DynamicEntryPointCommandGroup without adding additional code to these groups. Since we might want to add also an option to groups being completely deprecated (for example verdi computer configure), I think it is a better approach to use a custom command class for adding the deprecating logic for individual commands.

[sphuber]

I think it is a better approach to use a custom command class for adding the deprecating logic for individual commands.

Fair enough. The one downside currently though is that if you are deprecating a command, you have to manually add the cls=VerdiComman and add the import. This can be easily forgotten. But I think there might be a way to change the base command class in the VerdiCommandGroup. If that is possible, then I am fully on board with your solution.

What is the reason that you chose to use the parse_args as the hook for printing the deprecation message? Any particular reason?

[sphuber]

But I think there might be a way to change the base command class in the VerdiCommandGroup. If that is possible, then I am fully on board with your solution.

@agoscinski I had a look in the source code, and it is possible. You can set the Group.command_class to the VerdiCommand and then all subcommands should automatically use that class :+1:

[agoscinski]

What is the reason that you chose to use the parse_args as the hook for printing the deprecation message? Any particular reason?

It seemed to me most understandable point to insert this logic. The functions docstring

        """Given a context and a list of arguments this creates the parser
        and parses the arguments, then modifies the context as necessary.
        This is automatically invoked by :meth:`make_context`.
        """

and the code logic shows this clearly in Command https://github.com/pallets/click/blob/ac56c4551fcc5d4a60a66b252d6666bf58248e03/src/click/core.py#L1400-L1404 and we want to have the deprecated help before the parsing (the prompt should be created by handle_parse_result). I still must adapt the docstring then I think it becomes quite clear. Something like this

class VerdiCommand(click.Command):
    """Custom Command class to change logic how the deprecation message is handled."""

    def parse_args(self, ctx: click.Context, args: t.List[str]) -> t.List[str]:
        """Prints the deprecation message before the arguments are parsed and any parameters requests are prompted in the parents class implementation
        """
        if self.deprecated:
            # We are abusing click.Command `deprecated` member variable by using a
            # string instead of a bool to also use it as optional deprecated message
            echo_deprecated(
                self.deprecated
                if isinstance(self.deprecated, str)  # type: ignore[redundant-expr]
                else 'This command is deprecated.'
            )

        return super().parse_args(ctx, args)

[agoscinski]

Suggestion for squashed commit message (did not want to squash to make further review easier)

CLI: Enables deprecation warnings to be printed before any prompts (#6433)

`click.Command`s have a deprecated flag that results in a printed deprecation
warning on usage (and adaptions of help page). The `click.option` prompts are
however invoked before that warning is printed. Therefore we customize the
`click.Command` class introducing `aiida.cmdline.groups.VerdiCommand` to move
the printing logic to before the first prompt is shown.

I thought about replacing everywhere usage of decorators.deprecated_command with this solution, but needs some more thoughts and I don't want to block this PR further. The deprecation messages are a bit outdated, and the question is if we update them or remove these commands. I'll open an issue.

[sphuber]

I thought about replacing everywhere usage of decorators.deprecated_command with this solution, but needs some more thoughts and I don't want to block this PR further. The deprecation messages are a bit outdated, and the question is if we update them or remove these commands. I'll open an issue.

Fully agree, this can and should be done in a separate PR. We can leave the changes to the setup commands as you have already done them. Do you agree with my suggested change to the docstring?

[agoscinski]

Yes, wanted to add them, but you have already. Should I rebase?

[sphuber]

Weird, the warning that fails the doc build is:

/home/docs/checkouts/readthedocs.org/user_builds/aiida-core/envs/6433/lib/python3.11/site-packages/aiida/cmdline/groups/verdi.py:docstring of aiida.cmdline.groups.verdi.VerdiCommand.parse_args:1: WARNING: py:meth reference target not found: make_context

but we don't reference make_context in that docstring 😕

[agoscinski]

I think because it (the docstring) is inherited. Can I push a fix?

[sphuber]

Thanks a lot @agoscinski