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

Possibly remove defaulting values in flit-config.toml #254

Open
mikebentley15 opened this issue Mar 14, 2019 · 1 comment
Open

Possibly remove defaulting values in flit-config.toml #254

mikebentley15 opened this issue Mar 14, 2019 · 1 comment
Labels
documentation Involves touching documentation enhancement python Involves touching python code question tests Involves touching tests

Comments

@mikebentley15
Copy link
Collaborator

Feature Request

Describe the new feature:
When values are missing in flit-config.toml, much effort has been taken to give decent default values, such as taking g++, clang++ and icpc from the system PATH and handling missing ones gracefully. However, with issue #93 (Fortran support) and adding other languages, the configuration file is becoming much more complex. It is becoming increasingly difficult to handle giving intuitive defaults with a configuration file that is both backward compatible and handling new features.

I think moving forward, it is unmaintainable to handle missing values for compilers and compiler collections. The other fields are possible to have as default values, but trying to specify which fields are required and which are not may or may not be easy to do. This is something that needs to be addressed to make FLiT easy to use, understand, and maintain.

Suggested change:
I suggest making all fields required in the flit-config.toml. This is easier to specify to the user that everything is needed rather than trying to be clean which ones are or are not. This approach has some benefits:

  1. Easier to implement, with little possibility for mistakes
  2. Easier to document
  3. Could remove the graceful handling of missing compilers, and issue either a warning or error to the user that the compiler cannot be found or an invalid configuration.
  4. Could autogenerate flit-config.toml based on the user environment rather than hard-coded default values. This makes for potentially more user-friendly without making it opaque

There are some consequences of such an approach:

  1. More cumbersome, especially when using most of the values in the generated flit-config.toml. The configuration file becomes verbose.
  2. Not backward compatible. This is a breaking change without changing the major version number. (Or maybe it would be a good idea to make it a new major version release - support for multiple languages seem like a good reason for a version upgrade)
  3. Any new field added to the config file is an automatic breaking change, whereas before, we could just default to the old behavior before that field was introduced. Now, we would hopefully have a meaningful error or warning to the user that specifies a possible good value to add to the configuration file (making it again more complex).

Alternative approaches:
The main alternative approach is to pick and choose which values in the configuration file are required and which are not. It would have the following benefits:

  1. Allows for simpler configuration files
  2. New fields added to flit-config.toml would not necessarily constitute a breaking change. It would only be a breaking change if that added field were a required field.
  3. Could remove the graceful handling of missing compilers, and issue either a warning or error to the user that the compiler cannot be found or an invalid configuration.
  4. Could autogenerate flit-config.toml based on the user environment rather than hard-coded default values for the required fields. The non-required fields would have hard-coded default values (such as enabling MPI) which are specified in the default Toml file.

And the following consequences:

  1. More complicated to implement (although easier than what we have now I think)
  2. More complicated to document and understand
  3. Still would not be backward compatible to before where none of the fields are required

Currently, I believe the following fields (after #93 is accepted) would need to be required:

  • [dev_build]: all fields
  • [ground_truth]: all fields
  • [[compiler]]: all fields (except maybe required_flags, ld_flags, optimization_levels, and switches_list)
  • [[compiler_collection]]: all fields (except, I think the langs field is maybe not even necessary to have)

Discussion:
I am not fully convinced that my proposed approach is better than the alternative approach listed above. If you have an opinion or even another alternative approach, please discuss in this issue.

@mikebentley15 mikebentley15 added enhancement question python Involves touching python code documentation Involves touching documentation tests Involves touching tests labels Mar 14, 2019
@jjgarzella
Copy link
Contributor

I think this is the question we need to ask ourselves when deciding between your proposal and the alternative option: Are we going to be (semi-) regularly adding options to flit-config.toml in the future?

If yes, we should make only things that are absolutely essential required, for the breaking change effect. If not, then we can fix all the flags.

The answer honestly doesn't pop out to me at the moment, I want to continue to use FLiT and see how often needs for more options come up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Involves touching documentation enhancement python Involves touching python code question tests Involves touching tests
Projects
None yet
Development

No branches or pull requests

2 participants