-
Notifications
You must be signed in to change notification settings - Fork 58
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
ENH: Enable overriding ExperimentConfig parameters from container #283
Conversation
# Current ExperimentConfig parameter priority is as follows: | ||
# 1. Container | ||
# 2. CLI | ||
# 3. ExperimentConfig defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to have CLI as the highest priority level. Anytime someone goes through the pain of typing out something on the commandline, they probably really mean it ;-) You could achieve that by calling the update_experiment_config
in runner line 139.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble with that is that the user may need to use some container parameters to override ExperimentConfig
. For example, what should be the expected behaviour in this case?
# python runner.py --model MyContainer --batch_size 42
class MyContainer(LightningContainer):
def update_experiment_config(self, experiment_config):
experiment_config.tag = f"cool experiment with batch_size={self.batch_size}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we choose to call update_experiment_config()
before applying the CLI overrides to the container params, I'd at least make it a @staticmethod
to clarify that you should only use literals or read-only values. However, I think this option is very limiting (cf. example above).
In reality, we only need the model name first to instantiate the container. How about the following:
- Parse the runner arguments (
parser1
). - Instantiate container based on
--model
. - Override container params with remaining CLI args (
parser2
). - Call a
container.get_default_experiment_config()
method that instantiates anExperimentConfig
with whatever user-defined defaults (e.g.mount_in_azureml=True
). - Override experiment config with runner arguments (
parser1
). - [Optional?] Call
container.update_experiment_config(experiment_config)
now with access to the CLI-provided arguments (e.g. setexperiment_config.tag = f"cool experiment with batch_size={self.batch_size} on cluster {experiment_config.cluster}"
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dccastro not sure I follow the solution above, happy to follow up at voice if it helps. Overall I agree with @ant0nsc. CLI must have priority. And I think the logic with which we update arguments must be straightforward and easy to read otherwise it will become a nightmare. How often do you think we will be in the situation you describe? Is it something we already require in some of our configs?
@@ -147,6 +147,9 @@ def parse_and_load_model(self) -> ParserResult: | |||
apply_overrides(container, parser2_result.overrides) # type: ignore | |||
container.validate() | |||
|
|||
# Allow overriding ExperimentConfig params from within the container | |||
container.update_experiment_config(experiment_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think command line should overwrite what is the config file, therefore this should happen before apply_overrides
Closing as low-priority and we found no satisfactory solution at this stage. |
In a similar spirit to microsoft/InnerEye-DeepLearning#589, this PR enables overriding
ExperimentConfig
parameters (e.g.mount_in_azureml
,cluster
,num_nodes
, etc.) from aLightningContainer
. Theupdate_experiment_config()
method already existed, but was currently not being called by the hi-ml runner.