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

fix: gracefully exit if no profile exists #5874

Merged
merged 10 commits into from
Jan 30, 2023
Merged

fix: gracefully exit if no profile exists #5874

merged 10 commits into from
Jan 30, 2023

Conversation

zahid47
Copy link
Contributor

@zahid47 zahid47 commented Jan 26, 2023

fixes #5869

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.

cheers, thanks for your contribution @zahid47 !

Just one minor comment

aiida/cmdline/commands/cmd_daemon.py Outdated Show resolved Hide resolved
@ltalirz
Copy link
Member

ltalirz commented Jan 27, 2023

@sphuber not for this PR, but we may want to generalize the concept of commands that require an active profile (and exit if no profile is found without the command having to implement this)

@sphuber
Copy link
Contributor

sphuber commented Jan 27, 2023

@sphuber not for this PR, but we may want to generalize the concept of commands that require an active profile (and exit if no profile is found without the command having to implement this)

I agree. We should probably have a requires_profile decorator that should check before entering the command that at least one profile is defined and loaded.

@ltalirz
Copy link
Member

ltalirz commented Jan 27, 2023

That sounds like the right solution

@zahid47
Copy link
Contributor Author

zahid47 commented Jan 27, 2023

Am I on the right track? (I'm not very familiar with the codebase, and this is my first time "contributing" to open source, so any help is appreciated!!)

def requires_profile():
    """Function decorator to check that a command requires a profile to be defined and loaded.

    Example::

        @requires_profile()
        def mycommand():
            pass
    """

    @decorator
    def wrapper(wrapped, _, args, kwargs):
        """Check if a profile is set up and loaded. If not, echo a critical error and abort."""

        manager = get_manager()
        config = manager.get_config()

        # not checking using manager.get_profile() because we only care about at least one profile to be defined
        profiles = [profile for profile in config.profiles if not profile.is_test_profile]

        # do I need to check if the profile is loaded here somehow?

        if not profiles:
            echo.echo_critical('No profiles defined.')

        return wrapped(*args, **kwargs)

    return wrapper

@sphuber
Copy link
Contributor

sphuber commented Jan 27, 2023

You are not very far off @zahid47 . I think in general, most commands that would need such a decorator just need to check that a profile is loaded. So I would implement as the following:

def requires_loaded_profile():
    """Function decorator for CLI command that requires a profile to be loaded.

    Example::

        @requires_loaded_profile()
        def create_node():
            pass

    If no profile has been loaded, the command will exit with a critical error. Most ``verdi`` commands will
    automatically load the default profile. So if this error is hit, it is most likely that either no profile have been
    defined at all or the default is unspecified.
    """
    from wrapt import decorator

    @decorator
    def wrapper(wrapped, _, args, kwargs):
        """If daemon pid file is not found / empty, echo message and call decorated function."""
        from aiida.manage.configuration import get_profile

        if get_profile() is None:
            echo.echo_critical('No profile loaded: make sure at least one profile is configured and a default is set.')

        return wrapped(*args, **kwargs)

    return wrapper

Which basically just checks that a profile has been loaded. If you can add this in aiida/cmdline/utils/decorators.py and then add it to the status command, I think the test should pass and this PR should be good to go.

@zahid47 zahid47 requested review from ltalirz and sphuber and removed request for ltalirz January 27, 2023 19:50
@ltalirz
Copy link
Member

ltalirz commented Jan 28, 2023

Looks like the installation of the pre-commit hook itself failed? Never seen that before.

In any case, I believe this is good to merge.

Thank you for your contribution @zahid47, we will make good use of this decorator across the verdi cli. Welcome to the world of open-source contributors!

@zahid47
Copy link
Contributor Author

zahid47 commented Jan 29, 2023

Thanks so much for all the help and review! @sphuber and @ltalirz

@sphuber
Copy link
Contributor

sphuber commented Jan 30, 2023

Looks like the installation of the pre-commit hook itself failed? Never seen that before.

It's a problem with isort. Should be fixed now. Will update the pre-commit config in another PR first and then update/merge this one.

@sphuber sphuber self-requested a review January 30, 2023 09:57
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution @zahid47

@sphuber sphuber dismissed ltalirz’s stale review January 30, 2023 09:57

requested changes have been addressed

@sphuber sphuber merged commit ff3ae82 into aiidateam:main Jan 30, 2023
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 daemon status errors when no profile exists
3 participants