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

Should OmegaConfigLoader Support Cross-Environment Interpolation? Investigation Required #2465

Closed
noklam opened this issue Mar 24, 2023 · 8 comments
Assignees

Comments

@noklam
Copy link
Contributor

noklam commented Mar 24, 2023

Context:

Parent #2458 An user report that cross-environment interpolation is allowed for OmegaConfigLoader.

Description

I think it shouldn't be allowed as this can become confusing with the use of globals, user then have to check many files to confirm the source of parameters - and it's hard to reason about the resolution order.

Possible outcome

  1. This is fine, we should enable

  2. This isn't something we want, figure out what's need to be done to prevent this

           @merelcht: ok but why would you return a `dict` in `load_and_merge_dir_config` instead of a DictConfig object? I think that if we could keep the config data format internally a `DictConfig` object and only cast it to a dict when returning from `__getitem__` (it does then resolve variable interpolation etc). Oh and we should use `OmegaConf.to_container()` to cast it because it then nicely resolve variables etc. (unless you set `resolve=False`).
    

About interpolation: do we want to allow interpolation across envs? E.g.

--- 
# conf/base/parameters.yaml
server:
  host: localhost
  port: 80

--- 
# conf/local/parameters.yaml
client:
  url: http://${server.host}:${server.port}/
  server_port: ${server.port}

Because that's we currently (want to) do (I think)

Originally posted by @MatthiasRoels in #2458 (comment)

@noklam noklam changed the title Investigate if cross-env interpolation is possible and should we allow it Investigate if cross-env interpolation is possible and should it be allowed for OmegaConfigLoader Mar 24, 2023
@noklam noklam changed the title Investigate if cross-env interpolation is possible and should it be allowed for OmegaConfigLoader Investigate if cross environment interpolation is possible and should it be allowed for OmegaConfigLoader Mar 24, 2023
@noklam noklam changed the title Investigate if cross environment interpolation is possible and should it be allowed for OmegaConfigLoader Should OmegaConfigLoader Support Cross-Environment Interpolation? Investigation Required Mar 24, 2023
@merelcht
Copy link
Member

We need to verify what the current behaviour is. I would expect that cross-environment interpolation doesn't work. It also doesn't seem like desirable behaviour as it would be hard to reason about how and where the values of the templates come from.

@MatthiasRoels
Copy link

MatthiasRoels commented Mar 28, 2023

I agree with the fact that it could be hard to reason about where values come from. But on the other hand, allowing cross-env interpolation would mimic what the globals currently do in the TemplatedConfigLoader class right? Only in this version, you would need no globals pattern! One drawback of this of course is that you can only use variable interpolation within the same pattern (which differs from the globals pattern in behaviour in TemplatedConfigLoader).

@noklam
Copy link
Contributor Author

noklam commented Mar 28, 2023

I have done some testing and cross-env interpolation does not work currently @MatthiasRoels (expected as we never intend to allow it)

You are correct that with TemplatedConfigLoader the global scope is for all pattern, but this would only affect parameters.

All of options has some overlap area and we need to think what's the best way. One thing we don't want to see is that people start abusing global everywhere.

Allowing cross-env interpolation also make the paramters_global.yml redundant

@merelcht
Copy link
Member

It's true that globals with the TemplatedConfigLoader affect all environments, but the OmegaConfigLoader isn't meant to replicate this behaviour in the same way.

Our goals is to deprecate the legacy ConfigLoader and the TemplatedConfigLoader and make OmegaConfigLoader the new default. However, the OmegaConfigLoader was never meant to be a 1-1 replacement of either config loader. The solutions that exist today have evolved and been added over time, some by the core Kedro team, some from outside contributions. They were never purposefully designed, but rather had features added on to solve problem by problem.

With the OmegaConfigLoader we want to address user pain points with configuration in Kedro, but not by copying the existing solutions, but rather come up with solutions that solve those problems and optimise design. We also want to ensure this new config loader and potentially any future ones, are easy to extend so that users can add customisations if they must.

The current abstraction of the ConfigLoader and the TemplatedConfigLoader wasn't done properly and in fact the AbstractConfigLoader (which was meant as a base config loader to be extended) is completely useless.

I hope this explains the thinking that has gone into the OmegaConfigLoader so far and also the reason for it perhaps not seeming as a "complete" solution yet.

Thanks as always for the feedback and discussion @MatthiasRoels, hopefully we'll manage to deliver a solution that solves your need by the time we release 0.19.0 🙂

@noklam
Copy link
Contributor Author

noklam commented Mar 29, 2023

I am closing down this issue, unless someone think this is really needed. In that case we need to have separate discussion, I think this need to be discussed in a larger context how everything works together.

To summarize, Cross-env interpolation is not supported at the moment (expected).

@noklam noklam closed this as completed Mar 29, 2023
@noklam noklam self-assigned this Mar 29, 2023
@MatthiasRoels
Copy link

I agree that this should not be discussed in isolation but rather taken up into a broader config discussion!

@filpano
Copy link

filpano commented Mar 31, 2023

Sorry to flog a dead horse, but I'd like to point out some (personal) experience that I've had with cross-env interpolation and a few other related things.

I used to work a lot with the Java Spring ecosystem, and one of the central features there is "easy" and transparent application configuration.

To give a small sample of what's possible (I'll be using 'environment' and 'profile' interchangeably here, since that's the equivalent in the Spring world):

--- 
# Base 
# application.yaml
server:
 host: localhost
 port: 80

--- 
# Dev-profile specific
# application-dev.yaml

server:
 host: dev
client:
 url: http://${server.host}:${server.port}/

# URL after interpolation: http://dev:80

The idea is to have a base environment/profile for attributes that change less often and them being overridable in other environments/profiles.

Additionally, a grouping of environments is possible:

---
spring:
  profiles:
    group:
      prod: prod_data_lake,prod_settings

---
spring:
  config:
    activate:
      on-profile: prod_data_lake

client:
  url: 's3://...'
---
spring:
  config:
    activate:
      on-profile: prod_settings

client:
  partition_size: 256mb

# Starting the application with the 'prod' env enables the following:
# client.url: 's3://...'
# client.partition_size: 256mb

I realize that this is way out of scope of what Kedro wants to accomplish (since it's not meant for generalized application development), but perhaps these (IMO, really cool) features can inspire some future behaviour in Kedro.

On a slightly unrelated note, one of my first bookmarked pages in the Spring Documentation was a page documenting the order of variable interpolation.

I feel that a documentation page like this would be really helpful together with the new ConfigLoader. I was surprised more than a few times that e.g. environment variables did not have an effect on any defined placeholders in my catalog.

I hope this info can help with future discussions.

@MatthiasRoels
Copy link

What you describe here @filpano sounds similar to what Hydra has to offer. In earlier discussions, it was decided against a ConfigLoader based on Hydra (although I can’t remember why).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants