aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
436 stars 189 forks source link

Empty profile list when upgrading from v1.6.5 to `develop` #5116

Closed ltalirz closed 3 years ago

ltalirz commented 3 years ago

Describe the bug

This is the output of verdi profile list on my machine for aiida-core v1.6.5

$ verdi profile list
Info: configuration folder: /.../aiida_rmq/.aiida
  discover-hcofs
  fold-nodes
  mcloud_paper
* nanoporous_pipeline
  pyrene-mofs
  radames_django

When I update to develop, I get this

$ verdi profile list
$ verdi profile show

No error message, no warning, just an empty list.

In case it is relevant, I am using the AIIDA_PATH variable to specify the location of the AiiDA folder.

This is an excerpt from the config file

{
    "CONFIG_VERSION": {
        "CURRENT": 5,
        "OLDEST_COMPATIBLE": 5
    },
    "profiles": {
        "radames_django": {
            "AIIDADB_BACKEND": "django",
            "default_user_email": "radames.verdi@ope.ra",
            "AIIDADB_ENGINE": "postgresql_psycopg2",
            "AIIDADB_HOST": "localhost",
            "AIIDADB_PORT": 65256,
            "AIIDADB_NAME": "aiida_test_setup_django",
            "AIIDADB_USER": "aiida_SetupTestCase",
            "AIIDADB_PASS": "...",
            "AIIDADB_REPOSITORY_URI": "file:///.../aiida_rmq/aiida_radames_django",
            "PROFILE_UUID": "0646945891af44a1b0f91529dca7e714"
        },

Expected behavior

If the config file (or repo, ...) need to be migrated I would expect a warning and instructions on what to do.

Your environment

any thoughts @sphuber ?

P.S. When running a command that needs the db, I get

$ verdi process list
Critical:
Database schema version `1.0.48` is incompatible with the required schema version `1.0.49`.
To migrate the database schema version to the current one, run the following command:

    verdi -p nanoporous_pipeline database migrate

Is it possible that some changes in develop are causing AiiDA to need access to the database just in order to do a verdi profile list?

sphuber commented 3 years ago

Hmm, that's weird. So the config file is there just fine, but verdi profile list doesn't simply show anything? Did you run pip install -e .? Some dependencies have changed quite a bit, including click so that may be causing an exception. If that doesn't help, can you print some debug statements in the verdi profile list code to see to where it gets and when it craps out?

ltalirz commented 3 years ago

Thanks for the quick reply!

I did run pip install -e . but the printing is indeed the issue

$ ipython
Python 3.9.1 (default, Dec 11 2020, 06:28:49)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.20.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from aiida.cmdline.utils import defaults, echo

In [2]: echo.echo_info("test")

It was due to the fact that printing is now handled through the loggers.

For some reason, the config of the default profile (other than the first profile I showed above) had log levels explicitly defined

            "options": {
                "logging_django_loglevel": "DEBUG",
                "logging.aiida_loglevel": "WARNING",
                "logging.db_loglevel": "WARNING",
                "logging.sqlalchemy_loglevel": "WARNING",
                "caching.disabled": [
                    "abc.cde"
                ]

In 1.6.5, printing of the verdi cli was not affected by these levels. Now, I guess because of the hierarchy of loggers the aiida_loglevel also filters down to the cli.

It seems to me that this is an effect we may have overlooked and might want to change? I guess when someone changes the aiida_loglevel they typically don't want to affect the cli verbosity.

ltalirz commented 3 years ago

In my draft for the verbosity control, I had a VERDI_LOGGER and an AIIDA_LOGGER - I guess in your version there is only the AIIDA_LOGGER now?

One quick fix would be to reintroduce the VERDI_LOGGER above the AIIDA_LOGGER, and to route the echo commands through the VERDI_LOGGER.

sphuber commented 3 years ago

Ah yeah of course, you didn't see anything because your loglevel was set to WARNING? What is the reason for having it set to this? This way you also wouldn't see any report messages from workchains etc. I doubt many people have this. Setting verdi config unset logging.aiida_loglevel will return it to the default and everything will work as expected.

Then as to why we made the verbosity setting all of the logging: to me it is logical that the aiida_loglevel is the global setting and so controls everything. When specifying the verbosity on the command line we set this global value and not some local value that only influences the logging of the commands itself because one would expect that all output is controlled in a consistent way, also if it comes from module code. I would just keep this as is and add a warning to the release notes explaining the change telling people to run verdi config unset logging.aiida_loglevel if they have unexpected behavior, but I think the number of people that have it set to WARNING would be minimal because it essentially makes running workchains weird since there is no reporting whatsoever.

chrisjsewell commented 3 years ago

shouldn't the log level set at the CLI level, override anything set at the config level though?

sphuber commented 3 years ago

Yes, but only if you explicitly define a verbosity level through the option. I don't think the options default is applied by default. We could consider doing that, that -v/--verbosity has a default of REPORT by default and that is always applied even when not explicit defined. However, this would negate the functionality of setting a default log level on the config such that you don't have to specify the desired verbosity for each CLI invocation. I think last use case is still desirable.

ltalirz commented 3 years ago

Thanks for your thoughts guys!

Ah yeah of course, you didn't see anything because your loglevel was set to WARNING? What is the reason for having it set to this?

I don't remember, sorry.

This way you also wouldn't see any report messages from workchains etc. I doubt many people have this.

True.

to me it is logical that the aiida_loglevel is the global setting and so controls everything. When specifying the verbosity on the command line we set this global value and not some local value that only influences the logging of the commands itself because one would expect that all output is controlled in a consistent way, also if it comes from module code.

I don't agree. I can certainly envision scenarios where I might want to have a very high verbosity for the daemon logs but still want to be able to use the cli without being bombarded with messages.

Having a separate logger for the cli would also allow us to print a warning if someone sets the level to WARNING or above (which renders the cli unusable for the most part).

I think the number of people that have it set to WARNING would be minimal because it essentially makes running workchains weird since there is no reporting whatsoever.

I agree, this is not going to affect many people. The reverse case (having e.g. a DEBUG setting that now starts affecting cli use) might affect more.

We could consider doing that, that -v/--verbosity has a default of REPORT by default and that is always applied even when not explicit defined. However, this would negate the functionality of setting a default log level on the config such that you don't have to specify the desired verbosity for each CLI invocation. I think last use case is still desirable.

I would be in favor of this behavior.

Correct me if I'm wrong, but in my mind the use cases for changing logging verbosity permanently (via a configuration) are typically cases where you want to debug workchains etc. In these cases you typically don't want to also permanently change the verbosity of the cli output.

Changing cli output verbosity permanently is (I guess) a less common use case (difficult to say for sure since it was not possible until now). If we want to support it, I think we need to offer the option to configure it separately from the AiiDA logger, but I would also be ok with the above solution where one cannot change it permanently, only on demand via the -v option.

P.S. As a final thought, allowing people to change cli verbosity permanently might make it a tiny bit more difficult for us to help people with issues since it can result in unexpected output of commands due to this setting.