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

Inheritable Options #26

Open
PhilippWendler opened this issue Jan 23, 2019 · 5 comments
Open

Inheritable Options #26

PhilippWendler opened this issue Jan 23, 2019 · 5 comments

Comments

@PhilippWendler
Copy link
Member

Sometimes we want to create a new configuration based on an existing instance, but we do not want to copy all options from the existing one (this can be done with copyFrom), but only a select few options. There exists the method copyOptionFromIfPresent that can be used for this, but this has the disadvantage that the central code that creates the sub config needs to know all options that it should copy by name. Usually configuration options are declared in a decentralized manner.

We could solve this by adding a boolean inheritable to @Option and a method inheritFrom(Configuration) to ConfigurationBuilder. The difference between inheritFrom and copyFrom would be that the former affects only options with inheritable=true. Of course inheritance would be transitive. Probably we do not need to allow multi-inheritance.

@MartinSpiessl
Copy link

Wouldn't it be better to let the user decide which options shall be inherited inside the configuration file?
The mapping which options are inheritable and which are not could be different depending on the use case.
We already have the ::required annotation inside the configurations, e.g.:

limits.time.cpu::required = 900

We could do something similar like this:

limits.time.cpu::inheritable = 900

What is the advantage of inheritable options over splitting the original configuration into two files, one which contains what we want to inherit and the other which contains the rest?

@PhilippWendler
Copy link
Member Author

Wouldn't it be better to let the user decide which options shall be inherited inside the configuration file?

Probably it's hard for the user to know in which scenarios exactly the application will use inheritFrom. Applications can create lots of sub configs, and it would be disappointing for a user to see that ::inheritable has no effect for a particular option, because in the relevant code that creates the sub config inheritFrom is not used.

What is the advantage of inheritable options over splitting the original configuration into two files, one which contains what we want to inherit and the other which contains the rest?

I am not sure which scenario you describe. If users want to mark options as inheritable like in your proposal, they could move them to a different file instead and include that file everywhere, yes (so inheritance would not be important). But in my proposal, where application developers define which options are inheritable, how would a solution based on separate files look like?

@MartinSpiessl
Copy link

But in my proposal, where application developers define which options are inheritable, how would a solution based on separate files look like?

I guess I have not yet understood the use case that your proposal is trying to solve. From a user perspective, it would be very confusing if I have to track which options are declared as inheritable by the developer and which are not.

Probably it's hard for the user to know in which scenarios exactly the application will use inheritFrom.

The user would also need to know this in case of your proposal, right?

Back to what you stated at the end:

But in my proposal, where application developers define which options are inheritable, how would a solution based on separate files look like?

Let's think of an example. So your proposal would enable us to have a configuration, let's call it main.properties, like this:

misc.foo = true #not inherited
misc.bar = false #inherited
startSubProgramWithInheritance = path/to/subconfig.properties

The project using this configuration then can spawn a subprogram with a configuration that has all properties from path/to/subconfig.properties and misc.bar = false in case the developer implemented the handling of the option startSubProgramWithInheritance such that it uses inheritFrom. This would enable us to reuse subconfig.properties in several variants of main.properties which might differ in their values, both for the inherited and the other options. I could achieve the same thing by adding a variant of subconfig.properties in every of those cases. So your proposal saves me from generating those files.

Of course one partitioning into inherited and not inherited files might not be enough. When I look at code in CPAchecker currently, it is currently written in a way that there are several of those mappings depending on the configuration option.

For example in ConditionalVerifierAlgorithm, we have the following:

configBuild
            .copyFrom(globalConfig)
            .clearOption("analysis.asConditionalVerifier")
            .clearOption("conditional.verifier.verifierConfig")
            .clearOption("conditional.verifier.generatorConfig")
            .loadFromFile(verifierConfig)
            .setOption("analysis.entryFunction", pEntryFunctionName)

So for verifierConfig (corresponding property in the configuration file would be conditional.verifier.verifierConfig) the options that should not be inherited are (at least) the three options that are cleared with clearOption in the listing above.

In ParallelAlgorithm we the following:

ConfigurationBuilder singleConfigBuilder = Configuration.builder();
      singleConfigBuilder.copyFrom(globalConfig);
      singleConfigBuilder.clearOption("parallelAlgorithm.configFiles");
      singleConfigBuilder.clearOption("analysis.useParallelAnalyses");
      singleConfigBuilder.loadFromFile(singleConfigFileName);

singleConfigFilename here is fed from the list given by the property parallelAlgorithm.configFiles in the configuration file. The options that should not be inherited are at least those that are excluded with clearOption.

There are several other examples of this pattern, which could be solved better by your proposal, provided that all the partitions of the individual examples can be fused into two partitions of inherited and not inherited properties.

One could get rid of the clearOptions by restructuring the configurations, but that would be bad because the options that may not be set are then not documented and this is errorprone in the future. So I guess I understand why there are cases where restructuring the configurations is not a solution, and using clearOption to manually do this at various locations has a certain code smell. Hence your proposal could be a way to solve this, or did I get this wrong?

Here I actually looked at cases where we want all options except a few one, but you mentioned in your proposal that there are cases where we want "but only a select few options. " .

@PhilippWendler
Copy link
Member Author

Maybe the misunderstanding is that this feature proposal is not at all intended for cases where currently copyFrom is used, but for cases where copyOptionFrom[IfPresent] is currently used?

Combining copyFrom and inheritFrom would not make sense and would probably be forbidden.

But in my proposal, where application developers define which options are inheritable, how would a solution based on separate files look like?

I guess I have not yet understood the use case that your proposal is trying to solve. From a user perspective, it would be very confusing if I have to track which options are declared as inheritable by the developer and which are not.

Well, I would hope that the application developers would know for which options it makes sense that they should influence the sub config if specified in the main config. In CPAchecker's case, for example, these would be all those options that specify the semantics of the input task (e.g., mallocAlwaysSucceeds).

@MartinSpiessl
Copy link

Maybe the misunderstanding is that this feature proposal is not at all intended for cases where currently copyFrom is used, but for cases where copyOptionFrom[IfPresent] is currently used?

Yepp, thats it!

In CPAchecker's case, for example, these would be all those options that specify the semantics of the input task (e.g., mallocAlwaysSucceeds).

I see how it makes sense in this case. Though it would not be apparent that the booleaninheritable has this special meaning (semantics of input tasks) for CPAchecker and should not be misused for other stuff.
Could there also be a way of allowing a partitioning of the options that is decided by the user of java-common-lib? So e.g. instead of the boolean, a list of (user-defined) enum values. Then inheritFrom could specify from which of those partitions options should be inherited. In the example of CPAchecker, we would otherwise lose the information about which of the inherited options are actually needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants