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

Consider alternative log options #102

Open
ElliottKasoar opened this issue Apr 5, 2024 · 1 comment
Open

Consider alternative log options #102

ElliottKasoar opened this issue Apr 5, 2024 · 1 comment
Labels
enhancement New/improved feature or request

Comments

@ElliottKasoar
Copy link
Member

ElliottKasoar commented Apr 5, 2024

typer-config, which will be added in #100, includes options to dump the merged config, e.g.:

architecture: mace_mp
calc_kwargs: !!python/object:janus_core.cli.TyperDict
  value:
    model: small
device: cpu
log_file: !!python/object/apply:pathlib.PosixPath
- singlepoint.log
out_file: !!python/object/apply:pathlib.PosixPath
- NaCl-results.xyz
properties:
- energy
read_kwargs: !!python/object:janus_core.cli.TyperDict
  value:
    index: ':'
struct_path: !!python/object/apply:pathlib.PosixPath
- tests
- data
- benzene-traj.xyz
summary: !!python/object/apply:pathlib.PosixPath
- singlepoint_summary.yml
write_kwargs: null

The form of the paths isn't the nicest, but arguably this is a better (and much simpler to implement) summary, while things that require more processing e.g. start/end time, atomic structure details etc. could be added (or are already included) in the Python logging.

Probably not an priority immediate priority, but something to consider.

@ElliottKasoar
Copy link
Member Author

If we do want to use the dump, we need to deal with loading the Paths correctly, so the config can directly be reused.

A few options:

  • The simple alternative is to change the options to strings, since we don't actually use any of the advantages of paths at the moment (e.g. validation), but it's nice to have the option e.g. we probably should check the structure path exists (it'll still throw the same error, but typer might handle it more nicely)

  • I was initially thinking this may also be reason to try ruamel, but pyyaml is also an aiida-core dependency, so I think it's best to stick with it

  • The configs are still technically loadable, but not using the safe loader, so probably not the best idea to swap this out

  • Probably the nicest solution is we can do something similar how we changed the loader in Allow hyphens in Yaml configuration files #113 and swap out the default yaml dumper for one that uses the Path->str function that we already have

@ElliottKasoar ElliottKasoar added enhancement New/improved feature or request janus labels Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New/improved feature or request
Projects
None yet
Development

No branches or pull requests

1 participant