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 the %verdi IPython magics utility #5961

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 12, 2023

Fixes #5822

The aiida.tools.ipython.ipython_magics.AiiDALoaderMagics class
provides the verdi method, which allows to invoke verdi commands
from a Jupyter notebook, e.g., %verdi process list.

This was failing because it would invoke the verdi command directly
from the Python API. This would cause the --profile parameter to be
evaulated, which would attempt to load the current default profile. If
run from a notebook where the loaded profile was a temporary profile
that had been created on the fly, as is done in the tutorial notebooks
in the documentation, the command would fail. The reason is that the
temporary profile was created and loaded in memory, but not actually
added to the config. The --profile parameter, however, would
determine the default profile from the config, which would not be the
loaded temporary profile, and so it would attempt to load a different
profile, which is not allowed, with the default allow_switch being set
to False.

The solution is to not call the verdi command, but the subcommand,
thereby circumventing the --profile option. The subcommand name is
retrieved from the parsed command line arguments defined by the line
argument, where it should be the first element.

Docs: Fix the intro/tutorial.md notebook

The notebook creates and loads a temporary profile using the
SqliteTempBackend backend. This works fine as long as the profile is
only used in the current interpreter and no code is hit that checks the
profile is present in the Config. The reason is that although the
profile is loaded, it is created on the fly and not actually added to
the Config that is loaded in memory, nor is it written to the
config.json file on disk.

As soon as any code is called that will check the existence of the
temporary profile, through the config, it will fail. A good example is
when %verdi process list is called. At the end of the command, the
status of the daemon is checked, for which a DaemonClient instance is
constructed, which calls config.get_option('daemon.timeout', profile).
This call will validate the provided profile to check that it actually
exists, i.e., is known within the config, which is not the case, and so
a ProfileConfigurationError is raised.

The solution is to update the notebook to actually add the created
temporary profile to the config loaded in memory. Note that this could
have the undesirable consequence that if the config state is written to
disk, the temporary profile can be added to config.json. This will not
be automatically cleaned up. Since here it concerns a demo notebook that
will just be run on temporary resources anyway, it not being cleaned up
is not a problem.

Ideally there would be a utility for notebooks that creates a temporary
profile and actually adds it to the config, and cleans it up at the end
of the notebook. But it will be difficult to guarantee the cleanup in
all cases.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber.
If using %verdi -p <profile-from-config> what will happen after the change? It looks to me like it still will fail since the profile is not the temporary one in notebook, correct?

return verdi(shlex.split(line), prog_name='%verdi', obj=obj, standalone_mode=False) # pylint: disable=too-many-function-args,unexpected-keyword-arg

cmdline_arguments = shlex.split(line)
context = Context(verdi)
Copy link
Member

Choose a reason for hiding this comment

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

Can you give me an example of what this context will be. Is this the profile name passed to the verdi command when -p is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right to point this out, if the user actually calls %verdi -p some-profile node show that will fail. But even if we were to parse it correctly, it would still fail if the specified profile is other than the currently loaded one. I think this behavior should not be supported. The only reason for the %verdi magic, as I understand it, is to be able to use a verdi command for the currently loaded profile in a notebook.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Then I think we can raise an exception explicitly if the user pass -p.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to be careful and also check for --profile. But yeah, I can add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a check and raise if the profile parameter is specified.

@sphuber sphuber force-pushed the fix/5822/verdi-ipython-magic branch from 359bf5f to a0031c4 Compare April 13, 2023 09:22
@sphuber sphuber requested a review from unkcpz April 13, 2023 09:22
@sphuber sphuber force-pushed the fix/5822/verdi-ipython-magic branch from a0031c4 to bb936b1 Compare April 13, 2023 09:31
sphuber added 2 commits April 13, 2023 14:46
The `aiida.tools.ipython.ipython_magics.AiiDALoaderMagics` class
provides the `verdi` method, which allows to invoke `verdi` commands
from a Jupyter notebook, e.g., `%verdi process list`.

This was failing because it would invoke the `verdi` command directly
from the Python API. This would cause the `--profile` parameter to be
evaulated, which would attempt to load the current default profile. If
run from a notebook where the loaded profile was a temporary profile
that had been created on the fly, as is done in the tutorial notebooks
in the documentation, the command would fail. The reason is that the
temporary profile was created and loaded in memory, but not actually
added to the config. The `--profile` parameter, however, would
determine the default profile from the config, which would not be the
loaded temporary profile, and so it would attempt to load a different
profile, which is not allowed, with the default `allow_switch` being set
to `False`.

The solution is to not call the `verdi` command, but the subcommand,
thereby circumventing the `--profile` option. The subcommand name is
retrieved from the parsed command line arguments defined by the `line`
argument, where it should be the first element. A check is performed to
see if the user tried to specify a specific profile, in which case the
first element would be `-p` or `--profile`. This is not supported and an
exception is raised. If any other string is provided that is not a valid
subcommand, the `context.get_command()` call will raise a `UsageError`
just as if it would have been called directly on the CLI.
The notebook creates and loads a temporary profile using the
`SqliteTempBackend` backend. This works fine as long as the profile is
only used in the current interpreter and no code is hit that checks the
profile is present in the `Config`. The reason is that although the
profile is loaded, it is created on the fly and not actually added to
the `Config` that is loaded in memory, nor is it written to the
`config.json` file on disk.

As soon as any code is called that will check the existence of the
temporary profile, through the config, it will fail. A good example is
when `%verdi process list` is called. At the end of the command, the
status of the daemon is checked, for which a `DaemonClient` instance is
constructed, which calls `config.get_option('daemon.timeout', profile)`.
This call will validate the provided profile to check that it actually
exists, i.e., is known within the config, which is not the case, and so
a `ProfileConfigurationError` is raised.

The solution is to update the notebook to actually add the created
temporary profile to the config loaded in memory. Note that this could
have the undesirable consequence that if the config state is written to
disk, the temporary profile can be added to `config.json`. This will not
be automatically cleaned up. Since here it concerns a demo notebook that
will just be run on temporary resources anyway, it not being cleaned up
is not a problem.

Ideally there would be a utility for notebooks that creates a temporary
profile and actually adds it to the config, and cleans it up at the end
of the notebook. But it will be difficult to guarantee the cleanup in
all cases.
@sphuber sphuber force-pushed the fix/5822/verdi-ipython-magic branch from bb936b1 to 23705ab Compare April 13, 2023 12:46
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

LGTM. I also did a test locally and works as expected.

@sphuber sphuber merged commit b26c2d4 into aiidateam:main Apr 13, 2023
@sphuber sphuber deleted the fix/5822/verdi-ipython-magic branch April 13, 2023 21:21
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.

Issue with local load_profile and %verdi magic
2 participants