Config: Check current working directory for existing config dir

[sphuber]

Currently, the location of the configuration directory could only be controlled through the AIIDA_PATH environment variable. Here, the heuristic is updated to also consider the current working directory or any of its parent directories in case AIIDA_PATH is not set. This provides another easy way for users to maintain multiple separate AiiDA instances.

[danielhollas]

Very excited for this feature! 🤩 Gave it a quick test locally and seems to work.
Couple minor comments.

[danielhollas]

Also, please add .aiida/ to .gitignore.

[danielhollas]

LG apart from some nits, but I think one or more tests is warranted.

[danielhollas]

Cheers!

Though given the scope of the change, other people might want to chime in? Unless this change in behaviour was already discussed and agreed elsewhere.

[sphuber]

Thanks @danielhollas . This was more or less discussed offline with @giovannipizzi . Gio, do you want to have a quick look?

[giovannipizzi]

I think it's best if @GeigerJ2 checks, he did some extensive testing of various configurations/cases and found some issues (before this pr) and can give a first check (also to see if his issues are fixed by this pr)

[GeigerJ2]

Looks all good to me! Just added a few minor comments.

[GeigerJ2]

As mentioned by @giovannipizzi, I did some testing with AIIDA_PATH before, and found that if '.' is part of the list of paths (e.g. .:'some_path'), things break. While I'd say it's discouraged to set AIIDA_PATH to the cwd via '.', rather than providing the actual path, the following could still provide a safeguard:

    for base_dir_path in [path for path in environment_variable.split(':') if path]:
        if base_dir_path != '.':
            dirpath_config = pathlib.Path(base_dir_path).expanduser()
        else:
            dirpath_config = pathlib.Path.cwd().expanduser()

[sphuber]

What was the exact way in which this broke for you? I cannot reproduce it. For example:

In [2]: get_configuration_directory_from_envvar('.:/tmp')
Out[2]: PosixPath('/tmp/.aiida')

if there is no .aiida folder in the CWD. If there is such a folder, that path is used. So it seems to me like it is working as intended.

That being said, I notice that there is no validation whatsoever for the value in AIIDA_PATH. I wonder if, rather than trying to accommodate for all possible weird cases like ., we should really be validating and raising for invalid AIIDA_PATH values.
Validation could be:

1. Should be a string
2. When split on :, all elements should be absolute filepaths (allowing relative paths is for sure going to cause issues)
3. Each path (omitting the .aiida leaf if present) has to exist. If it doesn't exist, it is ignored.

Now this may be backwards incompatible, but I think if someone is using anything that does not respect these rules, chances are high that things can easily break anyway.

[GeigerJ2]

My issue was prior to this PR. I tested it with and without an empty .aiida directory present (as that was the use case), both in the first and second directory that was set via AIIDA_PATH, and using the verdi profile setup core.sqlite_zip command. This was part of the discussion on what is the fastest way to quickly explore an aiida-archive without polluting the main profile (before your actual verdi init PR). In both cases, nested .aiida/.aiida trees were generated, and, consequently, the exception:
FileNotFoundError: [Errno 2] No such file or directory: '.aiida/access/<profile>'
was thrown as the actual location became '.aiida/.aiida/access/<profile>'.

I just tried the same with the updated code, but still getting the same issue, while the pure Python function gives the same output as what you posted. I suppose there might be more things happening when runnnig verdi profile setup core.sqlite_zip?

However, I am also strongly in favor of doing some validation on AIIDA_PATH. Is it OK if I create a commit directly on this PR?

[sphuber]

I just tried the same with the updated code, but still getting the same issue, while the pure Python function gives the same output as what you posted. I suppose there might be more things happening when runnnig verdi profile setup core.sqlite_zip?

What did you run exactly? What is the value of AIIDA_PATH, if any? Which .aiida folders exist on your filesystem, and what is the current working directory when you call verdi profile setup core.sqlite_zip?

[sphuber]

However, I am also strongly in favor of doing some validation on AIIDA_PATH. Is it OK if I create a commit directly on this PR?

Since it seems to be an unrelated issue to the changes of this PR, I think it would be best to reserve that for a separate PR. Especially since adding validation would potentially be backwards incompatible, it would be best to not mix this here. I think we also need to discuss the coming team meeting if others agree to add the proposed backwards incompatibility.

[GeigerJ2]

Pasting the testing I did prior to this PR here for completeness (can later be moved to a separate PR):

---

Some notes:

• AIIDA_PATH was set to '.' from the parent folder in which the '.aiida' directory was supposed to be created (not that it should actually make a difference), and verdi profile setup core.sqlite_zip was run from this folder
• Manually added '.aiida' folders were always empty; apart from that I only had my global '~/.aiida'
• Warning = UserWarning: Creating AiiDA configuration folder '.aiida'
• ❌ = Breaks with FileNotFoundError due to nested '.aiida/.aiida'

---

1. AIIDA_PATH='.'

	No second path
First path -> '.aiida' present	❌ '.aiida/.aiida' tree in first path
First path -> '.aiida' absent	❌ Warning, then '.aiida/.aiida' tree in first path

---

2. AIIDA_PATH='<first_path>'

	No second path
First path -> '.aiida' present	✅ '.aiida' created in first path
First path -> '.aiida' absent	✅ Warning, then '.aiida' created in first path

---

3. AIIDA_PATH='.:<second_path>'

	Second path -> '.aiida' present	Second path -> '.aiida' absent
First path -> '.aiida' present	❌ '.aiida/.aiida' tree in first path	❌ '.aiida/.aiida' tree in first path
First path -> '.aiida' absent	✅ '.aiida' created in second path	✅ Warning, then '.aiida' created in second path

---

4. AIIDA_PATH='<first_path>:<second_path>'

	Second path -> '.aiida' present	Second path -> '.aiida' absent
First path -> '.aiida' present	✅ '.aiida' created in first path	✅ '.aiida' created in first path
First path -> '.aiida' absent	✅ '.aiida' created in second path	✅ Warning, then '.aiida' created in second path

---

So yeah, without '.', things work as expected, but with it, hell breaks loose. You're right - better to keep it separate and discuss it with the rest of the team 🚀

[sphuber]

Thanks for the detailed analysis @GeigerJ2 . I think this shows that validation of AIIDA_PATH is in order 😅 Is there anything else for this PR remaining, or can we merge this?

[GeigerJ2]

From my side, it's fine! Would have approved the merge directly, but not sure if I am ready to wield such power yet :D Thanks for the changes, I think they go in a good direction!