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

Enhancement: Compact key support #152

Closed
1 of 3 tasks
omry opened this issue Feb 20, 2020 · 9 comments · Fixed by #333
Closed
1 of 3 tasks

Enhancement: Compact key support #152

omry opened this issue Feb 20, 2020 · 9 comments · Fixed by #333
Labels
enhancement New feature or request
Milestone

Comments

@omry
Copy link
Owner

omry commented Feb 20, 2020

Compact keys

This will probably come in OmegaConf 2.1 as 2.0 is closed to new features at this time.

Given:

hydra:
  sweeper:
    params:
      budget: 100
      num_workers: 10

Allow the same config to be represented by:

hydra.sweeper.params:
  budget: 100
  num_workers: 10

Compatibility considerations

This changes the semantic meaning of a key with a dot, from a

foo.bar: 10

to:

foo:
  bar: 10

People using . in their keys now should be aware of it and ideally stop doing it (in 1.4 or 2.0).
OmegaConf 2.0 will add a deprecation warning that can be disabled by setting the environment variable OC_DISABLE_DOT_ACCESS_WARNING to the value "1".

To address this, there will be multiple opt out mechanisms (fully specification tbd).
some ideas:

  • Allow keys with dot by escaping the . : foo\.bar
  • Possibly allow file level opt out via a special header
  • Possibly allow API level opt out when loading the file or when constructing a DictConfig object.
@omry omry added this to the OmegaConf 2.0.0 milestone Feb 29, 2020
@omry omry added the enhancement New feature or request label Mar 24, 2020
@omry omry modified the milestones: OmegaConf 2.0, OmegaConf 2.1 Apr 6, 2020
@omry
Copy link
Owner Author

omry commented Apr 6, 2020

postponing for 2.1

@hadim
Copy link
Contributor

hadim commented Jul 6, 2020

I am using omegaconf to process Kubernetes YAML file. Some field (nvidia.com/gpu) requires to use a dot in its name. Example:

        # Request a number of CPUs and GPUs
        resources:
          requests:
            cpu: "16"
            nvidia.com/gpu: 1
          limits:
            cpu: "16"
            nvidia.com/gpu: 1

Those fields are decided at the level of the Kubernetes specification.

Any thoughts about this?

@omry
Copy link
Owner Author

omry commented Jul 6, 2020

Thanks for reporting.
for now suppress the warning with the environment variable.

I am planning to allow for some opt-out mechanisms before finalizing compact key support.

At the very least, you will be able to escape the dot in your yaml file:

        resources:
          requests:
            cpu: "16"
            nvidia\.com/gpu: 1
          limits:
            cpu: "16"
            nvidia\.com/gpu: 1

Is this sufficient for your use case?

You can check-out #155 and test this, it's already there.
Not to really use - it's not ready, just to verify that escaping like that really works as advertised.

@hadim
Copy link
Contributor

hadim commented Jul 6, 2020

Thanks I'll have a look.

@hadim
Copy link
Contributor

hadim commented Jul 24, 2020

Escaping won't do the job for Kubernetes specifications. If you turn this new behavior by default please:

  • Add a very clear warning by default
  • Provide an easy to use function to opt-out from it
    • even with such a function I can imagine case where a lib will turn the feature ON while another one want to have it OFF and it could create breaking issues.

Without this, we'll have to stick to 2.0. Also, it seems to be a pretty big breaking change, why not bump to 3.0 for this feature?

@omry
Copy link
Owner Author

omry commented Jul 24, 2020

Thanks for the feedback @hadim.
If you look at the original task, I am mentioning possible per-file opt out mechanism.
To be honest the pressure for this feature is reduced significantly with the introduction of packages in Hydra.

At this point this is still planned but it is not decided by any means.

@hadim
Copy link
Contributor

hadim commented Jul 24, 2020

Thanks @omry .

If the pressure is reduced and is less important then can I suggest you add it and turned it OFF by default? Would that work with you? Then people could turn it ON whenever they need it.

The first time I heard about this feature I found it super useful indeed but seeing the issue with the Kubernetes specification, I don't think I would like to have it ON by default anymore. And the reason goes beyond Kubernetes actually. I see Omegaconf as a super-powerful Python dict (so powerful that it should be in the std lib actually :-D) and this is the reason why I use it. So for what I am concerned about and the usage I have, Omageconf should not break Python dict compatibility.

That being said I understand this is very opinionated and very specific to my needs so no hard feeling if you turn it ON by default. I'll found a workaround.

@omry
Copy link
Owner Author

omry commented Jul 24, 2020

I will not land this feature unless I feel comfortable with it by adding sufficient escape hatches that support all use cases.
Once this gets more attention you will be able to comment on a concrete solution to see if it works for you.

@omry
Copy link
Owner Author

omry commented Aug 18, 2020

I am aborting this. reasoning in #332.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants