Improvement on loading and expanding YAML setting variables.

[hongyi-zhao]

• Added _expand_strings to handle environment and user variables in YAML.
• Introduced _load_yaml_file for loading YAML files with variable expansion.
• Updated _load_pmg_settings to use _load_yaml_file.
• Added tests for variable expansion, including $HOME, user-defined variables, and tilde (~) expansion.

[shyuep]

Maybe I am a bit unclear of the use case for this. What problem is this trying to solve?

[hongyi-zhao]

@shyuep
For improvement on loading and expanding YAML setting variables, see here:
https://github.com/hongyi-zhao/pymatgen/blob/d4e85a81c0004a858155cf4e39267dcdfc0d1df0/tests/core/test_settings.py#L47-L63

For adding VASP setting for the dftd4 vdW functional, see here:
https://github.com/hongyi-zhao/pymatgen/blob/d4e85a81c0004a858155cf4e39267dcdfc0d1df0/pymatgen/io/vasp/vdW_parameters.yaml#L8-L9

[shyuep]

I understand the proposed improvement. I am just not clear on the benefits over the current settings YAML loading. As far as I know, no one has ever had any issue with the current settings file with regards to things like path expansion etc.

[hongyi-zhao]

For example, in my case, the following configuration doesn't work without my PR due to the ~ not being expanded correctly:

werner@x13dai-t:~$ cat ~/.config/.pmgrc.yaml
PMG_DEFAULT_FUNCTIONAL: PBE_64
PMG_MAPI_KEY: <my-mapi-key>
PMG_VASP_PSP_DIR: ~/Public/hpc/vasp/pot/pmg_potcar

[shyuep]

When you create the yaml file using the pmg config command, all paths are absolute. Anyway, I don't have a problem with adding path expansion. But this is a lot of additional code for a simple variable expansion.

[hongyi-zhao]

When you create the yaml file using the pmg config command, all paths are absolute.

I agree with you: the path should be checked and correctly expanded with the absolute path when running the pmg config command and then stored in the yaml config file. Therefore, perhaps the logic I proposed here is not appropriate.

[hongyi-zhao]

@shyuep @janosh Anyway, I just canceled this PR on my local machine and kept it in my GitHub repo and here, so that everyone can discuss and deliberate on how to handle it.

The sub-commit in this PR for Add VASP setting for the dftd4 vdW functional. has been re-issued separately to facilitate the development workflow: #3899.

[janosh]

i think this PR is worth having. $HOME expansion makes the .pmgrc.yml more transferable if you keep the same directory structure relative to ~ for PMG_VASP_PSP_DIR e.g. between clusters.

[hongyi-zhao]

i think this PR is worth having. $HOME expansion makes the .pmgrc.yml more transferable if you keep the same directory structure relative to ~ for PMG_VASP_PSP_DIR e.g. between clusters.

However, this also brings up the following issue: when using pmg command to add configuration options, how should the corresponding values be added to the configuration file? Specifically, how to deal with the following problem: should the variables be expanded, or should the validity of the expanded variables be checked first and then still stored in the .pmgrc.yml file in their original variable form?

In any case, supporting as many possible forms as possible, including both the actual variable values and their unexpanded forms, should be the most flexible solution.

[shyuep]

In general, config files are supposed to be machine specific. It is reasonable to assume that a person would take the one time effort to set up .pmgrc.yaml for each cluster. Also, most people keep the same user name across different clusters, in which case even the absolute paths would be the same.

[janosh]

in which case even the absolute paths would be the same.

not the case e.g. for Lambda, GCP and AWS. they have different root directories preceding the user name for $HOME and $SCRATCH. would be nice if you could just carbon copy the pmgrc across HPC providers

[hongyi-zhao]

I reiterate my final point: In any case, supporting as many possible forms as possible, including the actual variable values and their unexpanded forms, should be the most flexible solution.

[shyuep]

I am fine with adding path expansion. But the solution implemented is just too complicated. There are very minimal settings in the pmgrc file. We do not need a recursive function to handle lists, dicts, etc.

I am a believer in implementing the simplest solution possible for practical problems. Personally, I am not convinced that being able to scp setting files over from machine to machine is a must-have. Like I said - it is a one time setup per machine, not something you do on a daily basis. But I will live with supporting path and user expansion for that esoteric purpose. However, that's all the support I am willing to add the the settings file. nested lists and dicts should not exist in the settings at the present moment. Until the day that that particular scenario actually manifests itself, we do not write code for hypothetical things.