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

Support optional comment header for printed configs #79

Closed
carmocca opened this issue Aug 11, 2021 · 3 comments
Closed

Support optional comment header for printed configs #79

carmocca opened this issue Aug 11, 2021 · 3 comments
Labels
enhancement New feature or request

Comments

@carmocca
Copy link
Contributor

Pitch

Allow passing a function that returns an str so the config generated with --print_config has a comment header with that string.

This would be useful to save which version was used to generate a config inside the config itself.

$ python script.py --print_config
# a comment
# which might span
# multiple lines
# and is configurable
foo: 1
@mauvilsa
Copy link
Member

Can you please explain more. Is the comment header one for the whole printed yaml? Why a function and based on what the string that returns changes?

Note that comments coming from docstrings are already included by using --print_config=comments. If an additional feature related to comments is added it would only be enabled by giving this option.

@carmocca
Copy link
Contributor Author

The idea is to be able to add some metadata to the config file for reproducibility. For Lightning, it could include the version when the config file was generated:

# Lightning version: v1.4.1
trainer:
  ...
model:
  ...

Is the comment header one for the whole printed yaml?

Yes

Why a function and based on what the string that returns changes?

Actually this is not a hard requirement so passing the string directly is okay

Note that comments coming from docstrings are already included by using --print_config=comments. If an additional feature related to comments is added it would only be enabled by giving this option.

This would be okay as long as

  • A --print_config=header is implemented, which would only include the header this PR requests. comments could include header + comments
  • A default print_config value can be programatically set for the ArgumentParser. This way --print_config can default to --print_config=header

Although you could also expect that if this metadata/header is passed to the parser, it should be included in the config by default

@mauvilsa mauvilsa changed the title Support optional comments for printed configs Support optional comment header for printed configs Aug 16, 2021
@mauvilsa mauvilsa added the enhancement New feature or request label Aug 16, 2021
@mauvilsa
Copy link
Member

This has been implemented in commit 00f42e0. Based on the described use case it didn't make sense to have a switch in --print_config so it is only a property of the parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants