-
Notifications
You must be signed in to change notification settings - Fork 184
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
Auto-load default AiiDA profile #6485
base: main
Are you sure you want to change the base?
Conversation
c8df507
to
eb0212c
Compare
Thanks @GeigerJ2, just putting the Discourse topic here for reference: https://aiida.discourse.group/t/should-we-load-the-default-profile-automatically/407 Two points off the top of my head:
|
Thanks for linking the Discourse thread, @mbercx. Fair enough. I didn't find a Not sure about the default for |
b67e356
to
eb0212c
Compare
Thanks @GeigerJ2 . I think it is worth discussing having an option to no longer force users to have to run
You don't need to determine the default manually though. If you look at the docstring of
We set it to |
Thanks for your comments @sphuber!
What is the source of your apprehension here? If users run Re the profile switching: I've done this quite often without issues, although admittedly that doesn't mean there can't still be bugs. One issue with switching profiles is of course that you no longer have access to data that was assigned to variables based on the previous profile, e.g.: In [1]: structure = load_node(1234)
In [2]: from aiida import load_profile
In [3]: load_profile('dev', allow_switch=True)
Out[3]: Profile<uuid='f9fdf6861b4d47f989e87ad3cbc4e715' name='dev'>
In [4]: structure The last line will raise a ClosedStorage: Storage for 'prod' [closed] @ postgresql://aiida:***@localhost:5432/aiida-lumi_prod_2 / DiskObjectStoreRepository: 0500319a13bb4c7ca70ecc15e0375205 | /hith/aiida-lumi/repositories/prod/container |
I don't have a problem with the Jupyter notebook. I would argue that instead we should just explicitly auto-load it in that very context. What I don't like about the current solution is that it is always loaded, even when using the API in a context that is not really used interactively by a user. For example some other client code that wants to use AiiDA's API. You don't always want the default profile to be loaded. This is a user specific thing, so we should enable that in user-specific contexts, e.g.
Another reason why I would argue for a more targeted solution. In the specific use case of Jupyter notebook, when it is started, a profile won't have been loaded yet, so you wouldn't even need to toggle this switch. Just toggling the switch by default all the way down in |
Hi guys, thanks a lot for getting back here! Yes, I agree it's something that is worth discussing/considering. Initially, I was thinking that it'd be nice to have it always on, but your argumentation, @sphuber, has convinced me that we better put it into the
Ah, interesting, thanks for the pointer! PS: Generally, I was in favor of always loading it, as I usually don't use jupyter notebooks for running stuff, but normal |
Note that for this use case, there is the
added to a script, and then made executable, will yield:
|
Though, not sure that I understand the issue here. The code is only called if the user would otherwise also have to manually run I'm not sure how to add the functionality so that it's specific for jupyter notebooks. I wouldn't make it part of the AiiDA notebook magic, because if people have to run: %load_ext aiida
%aiida then it defeats the purpose anyway. Will have a look how it's done for
That's very useful to know, thanks! Been actually wondering before what's the point of that, when I saw it ^^ |
The problem is that $ ipython
Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:34:09) [GCC 12.3.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.14.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from aiida.orm import Int
In [2]: Int()
---------------------------------------------------------------------------
ConfigurationError Traceback (most recent call last)
Cell In[4], line 1
----> 1 Int()
File ~/code/aiida/env/dev/aiida-core/src/aiida/orm/nodes/data/base.py:42, in BaseType.__init__(self, value, **kwargs)
39 except AttributeError:
40 raise RuntimeError('Derived class must define the `_type` class member')
---> 42 super().__init__(**kwargs)
44 self.value = value or self._type()
File ~/code/aiida/env/dev/aiida-core/src/aiida/orm/nodes/data/data.py:60, in Data.__init__(self, source, *args, **kwargs)
58 def __init__(self, *args, source=None, **kwargs):
59 """Construct a new instance, setting the ``source`` attribute if provided as a keyword argument."""
---> 60 super().__init__(*args, **kwargs)
61 if source is not None:
62 self.source = source
File ~/code/aiida/env/dev/aiida-core/src/aiida/orm/nodes/node.py:266, in Node.__init__(self, backend, user, computer, **kwargs)
259 def __init__(
260 self,
261 backend: Optional['StorageBackend'] = None,
(...)
264 **kwargs: Any,
265 ) -> None:
--> 266 backend = backend or get_manager().get_profile_storage()
268 if computer and not computer.is_stored:
269 raise ValueError('the computer is not stored')
File ~/code/aiida/env/dev/aiida-core/src/aiida/manage/manager.py:257, in Manager.get_profile_storage(self)
255 profile = self.get_profile()
256 if profile is None:
--> 257 raise ConfigurationError(
258 'Could not determine the current profile. Consider loading a profile using `aiida.load_profile()`.'
259 )
261 # request access to the profile (for example, if it is being used by a maintenance operation)
262 ProfileAccessManager(profile).request_access()
ConfigurationError: Could not determine the current profile. Consider loading a profile using `aiida.load_profile()`. My point is that it is not always obvious exactly how the storage backend can be requested and if we change the behavior that the default profile gets automatically loaded, that may break things. So although ultimately it might be fine to automatically load it always, I am first proposing that we try to limit the scope and only do it their where it is really needed, i.e., we reduce the surface area we impact.
I have to admit that I am also not quite sure how this can be done fully automatically. I would indeed include it in the |
The traceback you provide was also exactly what I had in mind, where it would be called.
Yeah, there might certainly be cases that I'm overlooking.
Sure, that makes sense! Fine for me to shelve the implementation of this PR for now for security. Thanks for the explanation on With "it defeats the purpose", I meant that if the user has to run: %load_ext aiida
%aiida rather than from aiida import load_profile
load_profile() they still need to add two lines of code, so it's kinda the same situation. Though, if |
Yeah, so %load_ext aiida
%aiida already automatically loads the profile. If we decide to add this feature there, then we can consider it done already :D |
Idea originally brought up by @mbercx. I thought this should be quick to do, so I just went ahead with it.
I can see how this could possibly lead to issues, as the profile is loaded silently in the background, without the user specifying it, so people might end up accidentally working within the wrong profile. Nonetheless, we can evaluate if it's an acceptable risk considering the gained benefit that one doesn't have to manually run
load_profile
every time. So, happy to discuss :)PS: Wondering if
get_default_profile
should be underaiida.cmdline.utils.defaults
or moved out ofcmdline
in a more general location?Edit: Also running
load_profile("<profile>")
leads to:though, this can be easily resolved by setting
allow_switch=True
.