OmegaConfigLoader returns Dict instead of DictConfig, resolves runtime_params properly

[noklam]

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Close #2458, #2466

This PR changes a lot how OmegaConfigLoader behavior although not many lines of code are changed.

kedro/kedro/config/omegaconf_config.py

Line 278 in fd42a5a

return dict(OmegaConf.merge(*aggregate_config))

The main changes are the resolving order and what's return by the config_loader

1. Lazily evalution -> Eager resolution as Dictionary

2. The merge with runtime_params is pushed to OmegaConfigLoader instead of KedroContext, this ensure that CLI params are updated properly

3. will ensure that the OmegaConfigLoader comply with AbstractConfigLoader and 2. will ensure that overriding parameters with CLI work as expected.

Note: This will only make parameters work, more work needs to be done to generalise it to all configurations. #2531

Development notes

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes

[noklam]

Need some more time to study, but for answering the 1st question. I don't think this is the correct way to do it, but currently, cross-env interpolation is blocked because of the dict cast (see more in the description) @MatthiasRoels

[noklam]

Background

#2458
@merelcht @AntonyMilneQB

The root cause of the issue has to do with the resolution order with OmegaConf, this is roughly what happen behind the scene:

1. config_loader["parameter"] - it needs to return a Dictionary
2. load_and_merge_dir_config (OmegaConfigLoader) - this is where the merging happen, ideally we need to use `OmegaConf.to_container(..., resolve=True) here
3. The result of 2 will merge context.extra_params recursively

In fact, it's not so different with TemplatedConfigLoader, the resolution order is the same:

1. Return a resolved dictionary - merge global dict with the Jinja template value
2. Result of 1 further merge with context.extra_params recursively

So it won't work too, the only thing works is a CustomTemplatedConfigLoader, see #1782, #1527 is something that we never solve.

1. Inject extra_params into self._config_mapping early
2. return resolved dictionary
3. merge 2 with context.extra_params

Can we use the same tricks for OmegaConfigLoader?
YES!! It works fine and we can still interpolate nested value (which isn't possible before). This will also works for catalog.yml, since it's generic to any config. The follow-up question will be, why do we need the nested update in context.params?

More details on how Current OmegaConfigLoader behave

When user do kedro run --params global:val, it will update the context.extra_params. In order to make the extra_params update all the reference, we need to keep it as a OmegaConf.DictConfig, because resolver has to be the last step. It also create the leaky abstraction problem because the behavior is not supposed to be ConfigLoader specific.

There are 2 options for step 2

A) Convert it to a Dictionary using OmegaConf.to_container(..., resolve=True)
B) Keep it as a OmegaConf.DictConfig

Option A seem to be the right thing to do, as it is consistent with what AbstractConfigLoader requires. However, overriding with params= will not work anymore because all the reference to ${global} is gone once the resolve=True happen.

Option B will make this works, but it creates other problems too. It is a leaky abstraction, as a result it only works with parameters, and you need special treatment to other things (e.g. logging). It also create a problem that parameter no longer return a Dict but a DictConfig

[noklam]

5b0bff8 Would solve all these problem

• OmegaConfigLoader parameters does not propogate properly #2458
• OmegaConfigLoader returns a DictConfig object instead of dictionary #2466
• And potentially remove the need of Only convert omegaconf DictConfig to primitive type for logging #2176

[merelcht]

5b0bff8 Would solve all these problem

Just to clarify something. In the earlier comment you mentioned:

There are 2 options for step 2
A) Convert it to a Dictionary using OmegaConf.to_container(..., resolve=True)
B) Keep it as a OmegaConf.DictConfig

Option A seem to be the right thing to do, as it is consistent with what AbstractConfigLoader requires. However, overriding with params= will not work anymore because all the reference to ${global} is gone once the resolve=True happen.

So I'm a bit confused as to 5b0bff8 solving all problems. Will it or will it not allow overriding with params= ?

[antonymilne]

Great work @noklam! I had reached similar conclusions to you also. In my mind there are just two different problems here:

1. whether and how to resolve the omegaconf DictConfig on this line
2. how to handle extra_params

On 1: both me (#1965 (comment)) and @MatthiasRoels (#2458) independently reached the same conclusion here: the current code looks wrong and the correct, non-leaky thing to do here is your option A above where you do to_container(resolve=True). I imagine the reason this wasn't done originally in #2176 is just that @merelcht didn't spot that you could set resolve=True in the to_container call? I'm sure this would solve the 3rd problem you list as "potentially solved" in your above comment also.

Now the 2nd problem of extra_params is a bit trickier. Here I previously thought the "correct" flow is the one that I suggested in #1965 (comment). This is somewhat unsatisfactory for two reasons though:

1. as I said there, it feels a bit circular/redundant to convert these objects to dicts, then back into dictconfig and then back into dict again. But in order to achieve something that's non-leaky I think that's what we need to do
2. something I didn't realise at the time I wrote that but you noticed with your global example: if extra_params are injected after to_container(resolve=True) then it's too late. As you also observed with TemplatedConfigLoader this actually isn't a new problem, but it would be nice if it did work

So what is the ideal solution here? I think exactly what you propose: since we already have runtime_params available in the configloader, let's merge them in before the call to to_container(resolve=True). I think there are a few potential objections here, but IMO none should stop us from making this change:

1. what is the point of the update in context.params? The original intention for this would have been for the interactive workflow where context.params can be changed on the fly without needing to do %reload_kedro. But as we've discussed before, this functionality is basically unknown and pointless, and I'd be very happy to remove it and just resolve everything eagerly instead
2. it means that --params can be used to inject variables into any config file, not just parameters. e.g. you could inject variables into your catalog through --params. This goes against the original intention of --params (that they are just for overriding parameters.yml) but is functionality that is sometimes called for (as in How to pass extra params to config loader since registration moved to settings.py in 0.18.x #1527) and so in my mind this is actually an improvement. If we really wanted to limit this functionality then we could put some conditional into the config loader itself so that the merging is only done when parameters are called for (rather than catalog etc.), but I don't like this
3. if and when we move the merging behaviour out of context.params entirely then any other configloaders would also need to perform the merging behaviour inside the config loader. This would include the existing ConfigLoader and TemplatedConfigLoader if they remain. But I think in future OmegaConfLoader should be the only one shipped with kedro, so then there's no need to worry about this. Anyone writing their own custom config loader would need to add it into their implementation though

Overall I think there's possibly a current objection (maybe @idanov would say this?) that the config loader should not be responsible for merging in runtime parameters. Or maybe --params wouldn't be the right name for the CLI flag any more if it's more general than that. But personally I think this would overall be an improvement and I don't see any better solution anyway.

So here's what I think the ideal final state would look like, without worrying right now about how to implement it or what's a breaking change etc.

• --params are processed with _split_params that does OmegaConf.from_dotlist and then to_container(resolve=False) to turn into a runtime_params dictionary (not dictconfig)
• pass runtime_params into configloader as you do here
• do to_container(resolve=True) inside omegaconfig loader including runtime_params as you do here
• ship just omegaconfigloader with kedro
• remove the merge from context.params entirely
• consider changing the name of --params if it's going to be used for more than just parameters

[antonymilne]

Does this definitely work when self.runtime_params is a dictionary and not a DictConfig? i.e. is it ok to not do OmegaConf.create(runtime_params) here? Looking through omegaconf source I'm pretty sure this is fine (only the first argument of OmegaConf.merge needs to be DictConfig) but please do check this!

[noklam]

I checked with the source code, merge can take any primitive type I think it's fine here.

    def merge(
        *configs: Union[
            DictConfig,
            ListConfig,
            Dict[DictKeyType, Any],
            List[Any],
            Tuple[Any, ...],
            Any,
        ]

[antonymilne]

Probably nicer to do runtime_params=runtime_params or {} above instead?

[antonymilne]

Or even better AbstractConfigLoader should actually be the class responsible for this defaulting behaviour, i.e. it should do self.runtime_params = runtime_params or {}.

[noklam]

100% agree with this.

[noklam]

@AntonyMilneQB Maybe slightly off topic. #2481 but in my mind this is the more correct way to do in long term, but should be discussed separately as I think we need a temporary solution.

[antonymilne]

Related to all this, we also need to think about this bit of code:

        if len(aggregate_config) == 1:
            return list(aggregate_config)[0]

As it stands if we do 5b0bff8 then it will still return an unresolved DictConfig if there's only one file in aggregate_config. I don't actually know what this special len == 1 case is for - the code should work even when len(aggregate_config) == 1 without any special treatment? @merelcht @noklam

# Works fine, so no need for `len(aggregate_config) == 1` case
OmegaConf.merge(*[OmegaConf.create({1: 2})])

# Doesn't work fine, hence need for `if not aggregate_config` case
OmegaConf.merge(*[])

[merelcht]

I don't exactly remember right now why I did this

if len(aggregate_config) == 1:
return list(aggregate_config)[0]

but there was definitely a good reason. There was something about just 1 config file not working the same as multiple.

[noklam]

@AntonyMilneQB I had the same comment before, I haven't really tested it so I am not sure here.

[merelcht]

LGTM! Thanks for getting to the bottom of this ⭐

One minor comment: in the tests it might be better to not refer to "global" but instead just a random parameter name, like test_split or whatever. Now it seems like this functionality is specifically for "globals" like what we have in TemplatedConfigLoader, but it's actually different.

[merelcht]

That's also just for the OmegaConfigLoader right?

[noklam]

That's correct, I will make changes to this

[noklam]

I change templated_param -> interpolated_param to align with the OmegaConf terminology.

I have changed the global name, but I think we need to come up with a better name since we are calling it global with OmegaConfigLoader, i.e.parameters_global.yml is what suggested in the doc.

[merelcht]

Suggested change

	"""Make sure `catalog`, `credetials` won't get updated by runtime_params"""
	"""Make sure `catalog`, `credentials` won't get updated by runtime_params"""

[antonymilne]

This is nearly there but I think there's a bug with it unless I'm missing something: what happens if you have a single parameters.yml file and want to use runtime_params?

As part of a separate PR I think we should revert #2378 (but keep test_no_DictConfig_in_store as it's a good regression test) and #2176.

[antonymilne]

Personally I'd put all this as bug fix rather than major improvement.

[noklam]

It's unclear to me if this is part of the official support feature. My reasoning go as below:

We never fixed the TemplatedConfigLoader as we promised we need some more thinking for 0.19. If this is considered a bug fix, I don't see why we can't fix this for TemplatedConfigLoader in 0.18.x and requires users to sub-class it instead.

In fact, TemplatedConfigLoader doesn't suffer from the residual problem so it can be safely used for all configurations. There is nothing wrong with merging the runtime_params as this is exactly what we are doing now.

#1782 (comment)

[antonymilne]

This is a fair comment I think for the "interpolated parameters" point, but I still think the dict vs. DictConfig point should be under bug fix.

[noklam]

@antonymilne I agree with this and I will move it to bug fix.

[antonymilne]

I think the flow here is a little unclear (too many different return cases) and, more importantly, unless I'm missing something there's actually a bug: if you have a single element in aggregate_config (i.e. one parameters file) then runtime_params don't get merged in.

This would be solved if we removed lines 276-279. I know @merelcht said there was some reason to pick out the len == 1 case before but I don't see what this could be now that we always want to do to_container(resolve=True).

[noklam]

I would be keen on removing it, unless we have a specific failing case. I try remove line 278-279 and all test case passed.

Should we separate this into another issue? Removing Line 276-277 will cause a few tests fail, I suspect it's also link to #2556 and some refactoring may be needed.

[merelcht]

I'm happy for those lines to be removed if everything is still working fine. I don't remember why I added it, so really should have left a comment 😅

[antonymilne]

I think we should remove line 278-279 here since as I understand it at the moment they are not just unnecessary but following this change will actually be causing a bug (would be good if you could check this): if you just have a single parameters.yml file, the runtime_params don't get merged in.

Lines 276-277 don't actually cause a bug I think, so if removing them makes some tests fail and it seems related to #2556 then let's leave those here for now and try to remove them in that PR.

[noklam]

As part of a separate PR I think we should revert #2378 (but keep test_no_DictConfig_in_store as it's a good regression test) and #2176.

Agree with this I can open an issue, it will be redundant.

[antonymilne]

Thank you for fixing it! Do take a look at my most recent comments but basically I think this is good to go 🙂 Would be nice to follow up with #2556 and the reversions I mentioned soon though just to get it all cleared up.