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 config list errors when no profile is configured #5920

Closed
2 tasks done
ltalirz opened this issue Mar 8, 2023 · 8 comments · Fixed by #5921
Closed
2 tasks done

verdi config list errors when no profile is configured #5920

ltalirz opened this issue Mar 8, 2023 · 8 comments · Fixed by #5921

Comments

@ltalirz
Copy link
Member

ltalirz commented Mar 8, 2023

Describe the bug

When no profile is configured, I get

$ verdi config list
Traceback (most recent call last):
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/aiida/common/extendeddicts.py", line 51, in __getattr__
    return self[attr]
KeyError: 'profile'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/path/to/aiida/aiida/bin/verdi", line 8, in <module>
    sys.exit(verdi())
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/aiida/cmdline/commands/cmd_config.py", line 43, in verdi_config_list
    profile: Profile = ctx.obj.profile
  File "/path/to/aiida/aiida/lib/python3.9/site-packages/aiida/common/extendeddicts.py", line 54, in __getattr__
    raise AttributeError(errmsg)
AttributeError: 'AttributeDict' object has no attribute 'profile

Steps to reproduce

Run verdi config list when no profile is configured

Expected behavior

The command should exit gracefully, and probably even show the available options

Your environment

  • Operating system [e.g. Linux]: Linux
  • Python version [e.g. 3.7.1]: 3.9
  • aiida-core version [e.g. 1.2.1]: main

Other relevant software versions, e.g. Postres & RabbitMQ

Additional context

@sphuber This is one of a class of issues that have to do with cli behavior when no profile is configured.
This is currently not tested since the profile is always already configured.
We may want to add a few "profile-free" tests for commands that we expect to still work when no profile is around.

@ltalirz ltalirz added type/bug priority/nice-to-have good first issue Issues that should be relatively easy to fix also for beginning contributors labels Mar 8, 2023
@sphuber
Copy link
Contributor

sphuber commented Mar 8, 2023

@sphuber This is one of a class of issues that have to do with cli behavior when no profile is configured.
This is currently not tested since the profile is always already configured.
We may want to add a few "profile-free" tests for commands that we expect to still work when no profile is around.

Fair point. I already fixed a similar issue you reported some time ago #5543 . The infrastructure to easily test these is there, we just have to come up with the list of commands to test like this. I will start a PR and we can add them there.

@sphuber sphuber removed the good first issue Issues that should be relatively easy to fix also for beginning contributors label Mar 8, 2023
@sphuber
Copy link
Contributor

sphuber commented Mar 8, 2023

Turns out adding a test is not so easy. The problem is that the ctx.obj.profile object is not set when no profile is available. This is done by the custom VerdiContext which is invoked when verdi is invoked. However, in the testing we use the run_cli_command fixture, which doesn't actually go through verdi but goes straight to the subcommand, thereby circumventing the initialization of ctx.obj through the VerdiContext. This is "fixed" by having run_cli_command build the ctx.obj manually and pass it to the CliRunner.invoke. The problem then is, however, that the test will always pass, so it is useless as a regression test.

One solution would be to run the test through an actual subprocess. I started this approach in #5846 . But then we cannot use the empty_config fixture, as that only operates in memory 😖

Long story short, I removed the good-first-issue label for now.

@ltalirz
Copy link
Member Author

ltalirz commented Mar 8, 2023

Thanks for the investigation - indeed, in that case not a good-first issue ;-)

This is "fixed" by having run_cli_command build the ctx.obj manually and pass it to the CliRunner.invoke

Could we just tell run_cli_command not to set ctx.obj.profile for such tests?

@ltalirz
Copy link
Member Author

ltalirz commented Mar 8, 2023

Anyhow, it's not a priority; no need to lose a lot of time over this.
Once #5846 is ready for review, let me know

@unkcpz
Copy link
Member

unkcpz commented Mar 8, 2023

I just run into the same issue, is #5846 will fix this? Regardless of the test, adding @decorators.with_dbenv() for verdi_config_list should fix this. Is a separate PR welcome?

@ltalirz
Copy link
Member Author

ltalirz commented Mar 8, 2023

verdi config shouldn't require the dbenv though, should it?

@unkcpz
Copy link
Member

unkcpz commented Mar 8, 2023

verdi config shouldn't require the dbenv though, should it?

I am not sure, I think it depends on if there are any config settings store in the DB, if not it shouldn't require.

@sphuber
Copy link
Contributor

sphuber commented Mar 8, 2023

I just run into the same issue, is #5846 will fix this? Regardless of the test, adding @decorators.with_dbenv() for verdi_config_list should fix this. Is a separate PR welcome?

No, the command definitely doesn't need the storage backend and so shouldn't have the decorator.

The issue is not fixed by #5846 but I am working on a fix that requires that PR first. Almost done and will open the PR with the fix soon, but we will need to review/merge #5846 first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants