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

Extend path resolution in configuration files #715

Closed
davidorme opened this issue Jan 31, 2025 · 0 comments · Fixed by #716
Closed

Extend path resolution in configuration files #715

davidorme opened this issue Jan 31, 2025 · 0 comments · Fixed by #716

Comments

@davidorme
Copy link
Collaborator

davidorme commented Jan 31, 2025

Way back when, we added code to resolve paths in configuration files. This is partly so that relative paths are always resolved relative to the specific config TOML they come from, and partly because VE needs resolved paths. This was implemented in #269 - but that PR only handles resolution of file paths in [core.data.variable] blocks and specifically notes that this is something we may need to revisit.

That time is now - the PlantsModel needs to load PFT definitions and forcing that through the data object is extremely heavy handed and will create a bunch of single use constant "variables" floating around in the data object.

The issue here is that the config file values from TOML have a very limited number of types, so paths are just strings, and we only want to try and resolve the strings that are paths and not other things.

  • A clever approach would be to use something that types path settings in a detectable way. We could add "I am a path" properties to our JSON Schema or possibly replace our schema setup with pydantic, which would be cleaner and more pythonic. The problem though is that we are resolving paths in arbitrary chunks of config before combining and validating them. So it's not clear how we'd match values to the schema.

  • A more pragmatic approach right now is to use the config key name to identify paths. We update the _resolve_config_paths function such that:

    • It walks over the entire config file contents,
    • looking for key, value pairs where key.endswith('_path') and isinstance(value, str)
    • and updates those values with the existing resolution mechanism in that function.

def _resolve_config_paths(config_dir: Path, params: dict[str, Any]) -> None:
"""Resolve paths in a configuration file.
Takes the path of a directory containing a given configuration file and resolves any
file paths in the configuration file contents, relative to that file location.
Todo:
At present, this only targets `core.data.variable` configuration entries and may
want to resolve additional paths in the future.
Args:
config_dir: A folder containing a configuration file.
params: A dictionary of contents of the configuration file, which may contain
file paths to resolve.
"""

This does:

  • involve updating the [core.data.variable] spec to use file_path and not file, but I think it is worth having a single clear pattern.
  • mean we have to avoid using keys ending in _path for other uses, but that seems reasonable.
@davidorme davidorme linked a pull request Jan 31, 2025 that will close this issue
8 tasks
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 a pull request may close this issue.

1 participant