Use only one global var for marking config folder tree

[unkcpz]

The problem is found when I worked on #6609. If the import of DAEMON_DIR moved to outside the function, the global var set in module scope and in test contest will not be reset when function is loading.

The goal for a better design is to reduce the use of global variable. Now it is only the glb_aiida_config_folder (the original AIIDA_CONFIG_FOLDER). Other three are derived from this one. This lower the possibility of directly access the global variable without noticing its change.

• use a class to derive daemon dir and daemon log dir instead of using 4 global vars.
• Remove usage daemon dir, log dir, access control dir from other modules.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 12 lines in your changes missing coverage. Please review.

Project coverage is 77.90%. Comparing base (ef60b66) to head (6e26eaf).
Report is 146 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/manage/configuration/profile.py	16.67%	5 Missing ⚠️
src/aiida/manage/tests/pytest_fixtures.py	0.00%	3 Missing ⚠️
src/aiida/cmdline/commands/cmd_profile.py	0.00%	2 Missing ⚠️
src/aiida/engine/daemon/client.py	77.78%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6610      +/-   ##
==========================================
+ Coverage   77.51%   77.90%   +0.40%     
==========================================
  Files         560      567       +7     
  Lines       41444    42178     +734     
==========================================
+ Hits        32120    32856     +736     
+ Misses       9324     9322       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[GeigerJ2]

If basedpyright is happy with the code, there is probably nothing I can add :D
Looks good to me! Just a few minor comments.

[unkcpz]

Thanks for the review and great suggestion @danielhollas @sphuber
I addressed all what you mentioned, and ready for another review I think.

[unkcpz]

Hi @danielhollas, please have a look at this when you get time. This is one of the fixes for a random failed test of #6620.

Besides the naming of the get/set methods, it is ready from my side. I prefer to have the method name get/set instead of AiiDAConfigDir.get_configuration_directory().

[danielhollas]

@unkcpz Thanks, I'll have a look today.

One suggestion, it would help if the unrelated typing changes were extracted to a separate PR. But if it would be too much extra work don't worry about it.

Besides the naming of the get/set methods, it is ready from my side. I prefer to have the method name get/set instead of AiiDAConfigDir.get_configuration_directory().

I agree with the rename.

[unkcpz]

One suggestion, it would help if the unrelated typing changes were extracted to a separate PR. But if it would be too much extra work don't worry about it.

Not too many, and I agree it is better to not contaminate the major changes with fix type checking too much. I revert the changes in configuration/profile.py which fixes are not necessary.

I agree with the rename.

Renamed.

[unkcpz]

Suggested change

	"""Singleton for setting and getting the path to configuration directory."""
	"""Global access class that hold the attribute for the absolute path of AiiDA configuration folder. The class method ``set`` and ``get`` are provided to mutate and access the location of configuration folder."""

Look back and the implementation, I think it is not the actually singleton pattern, which only instantiate the object once and only once for entire program.
The old change I put was having a _glb_aiida_config_folder variable and it seems fit more with the global variable of the module and the module in python is a singleton so the global variable will therefore shared over the entire program.
In the current implementation, it is AiiDAconfigDir._glb_aiida_config_folder the class attribute plays this global variable role.

I think the new docstring above makes more sense for the class.

[sphuber]

It is indeed definitely not a singleton. I think I left a comment before asking why you went with a class and a class attribute to store the global config directory, but I cannot find it nor a response. I still don't really see why this requires a class? Wouldn't it be perfectly fine to simply do:

_AIIDA_CONFIG_FOLDER: pathlib.Path | None = None

def get():
    global _AIIDA_CONFIG_FOLDER
    return _AIIDA_CONFIG_FOLDER

def set(aiida_config_folder):
    global _AIIDA_CONFIG_FOLDER
    _AIIDA_CONFIG_FOLDER = aiida_config_folder
    _create_instance_directories(aiida_config_folder)

[unkcpz]

I think I implemented in this way and then agree with @danielhollas it would be better to move as class attribute to the class.
Logically it is the same as current implementation, just the difference of how codes are organized and how the set/get functions are called.

With class and attributes, the class server as the global variable with the module, and get/set are called by:

from aiida.manage.configuration.settings import AiiDAconfigDir

AiiDAconfigDir.set("/tmp/.aiida")
config_folder = AiiDAconfigDir.get()

with global variable of module, get/set are called by:

from aiida.manage.configuation import settings

settings.set_configuration_folder("/tmp/.aiida")
config_folder = settings.get_configuration_folder()

I personally prefer the AiiDAConfigDir way which seems more compact.

[sphuber]

Sure, in the end it is a matter of taste. I just wanted to ask because I don't like this general attitude that "everything must be a class". There are plenty of situations where creating an entire class for a simple function is not necessary.

[unkcpz]

I don't like this general attitude that "everything must be a class".

Yep, I totally agree with this ;)
I am not yet on the haskell side that "everthing must be a function" but I am definitely fan of classes.

[unkcpz]

There is another advantage of using global keyword which is I have to put the # PLW0608 to silent the mypy. It makes it easier to find where we use the global variables. Usually those are places that may not thread-safe.

@danielhollas want to hear what you think, I think after separate the AiiDAConfigPathResover out, it seems better to not using AiiDAConifgDir to encapsulate it as @sphuber suggested?

[danielhollas]

I am in favour of keeping the AiiDAConfigDir class, I like how it encapsulates the global variable. Otherwise, it's too easy to modify the global variable directly, without going through the setter. (Yes, I am aware that one can modify the class variables directly as well, but not without intentional effort.)

[danielhollas]

This looks great! Thanks for sticking with this @unkcpz, very nice cleanup and the settings.py file is now much easier to follow.

[danielhollas]

I am in favour of keeping the AiiDAConfigDir class, I like how it encapsulates the global variable. Otherwise, it's too easy to modify the global variable directly, without going through the setter. (Yes, I am aware that one can modify the class variables directly as well, but not without intentional effort.)

[unkcpz]

Thanks for the review and discussion! @sphuber @danielhollas @GeigerJ2