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

[Bug] Unable to delete an item from ListConfig #2477

Open
jbaczek opened this issue Nov 23, 2022 · 5 comments
Open

[Bug] Unable to delete an item from ListConfig #2477

jbaczek opened this issue Nov 23, 2022 · 5 comments
Labels
bug Something isn't working
Milestone

Comments

@jbaczek
Copy link
Contributor

jbaczek commented Nov 23, 2022

🐛 Bug

Description

I'm unable to delete an item from a ListConfig using the command line. Ex. using the list indexing syntax config.list.0 a list index is parsed as a string which raises a TypeError in omegaconf's ListConfig.
The bug is caused by lines 367-368 in hydra/_internal/config_loader_impl.py file.

node = OmegaConf.select(cfg, key[0:last_dot])
del node[key[last_dot + 1 :]]

key is a string indexing path within a config. In my case it ends with 0 because i want to delete 0th element of the list. The node retrieved is of type ListConfig which accepts ints and slices as indices. But key[last_dot + 1 :] is clearly a string.

Checklist

  • [ x] I checked on the latest version of Hydra
  • [ x] I created a minimal repro (See this for tips).

To reproduce

Assemble a config that contains a list inside. Ex:

config:
    x:
      - elem_1
      - elem_2

python my_app.py -c job ~config.x.0

** Stack trace/error message **

Traceback (most recent call last):                                                                                                                                                                                   File "/opt/conda/lib/python3.8/site-packages/hydra/_internal/utils.py", line 213, in run_and_report                                                                                                                  return func()                                                                                                                                                                                                    File "/opt/conda/lib/python3.8/site-packages/hydra/_internal/utils.py", line 400, in <lambda>                                                                                                                        lambda: hydra.show_cfg(                                                                                                                                                                                          File "/opt/conda/lib/python3.8/site-packages/hydra/_internal/hydra.py", line 195, in show_cfg                                                                                                                        cfg = self.compose_config(                                                                           
  File "/opt/conda/lib/python3.8/site-packages/hydra/_internal/hydra.py", line 594, in compose_config    
    cfg = self.config_loader.load_configuration( 
  File "/opt/conda/lib/python3.8/site-packages/hydra/_internal/config_loader_impl.py", line 141, in load_configuration                                                                                             
    return self._load_configuration_impl(                                                                
  File "/opt/conda/lib/python3.8/site-packages/hydra/_internal/config_loader_impl.py", line 265, in _load_configuration_impl                                                                                       
    ConfigLoaderImpl._apply_overrides_to_config(config_overrides, cfg)                                                                                                                                             
  File "/opt/conda/lib/python3.8/site-packages/hydra/_internal/config_loader_impl.py", line 357, in _apply_overrides_to_config                                                                                     
    del node[key[last_dot + 1 :]]                                                                                                                                                                                  
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/listconfig.py", line 364, in __delitem__                                                                                                                  
    del self.__dict__["_content"][key]                                                                   

Expected Behavior

I should be able to remove element from a list using ~config.list.<index> syntax

System information

@jbaczek jbaczek added the bug Something isn't working label Nov 23, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented Nov 26, 2022

Thanks for the report @jbaczek.

This reminds me of the open omegaconf issue omry/omegaconf#864

node = OmegaConf.select(cfg, key[0:last_dot])

When cfg is a ListConfig, I guess we should be able to coerce the key to an int.
(For DictConfig, coercion is less desirable because cfg could have e.g. a string key "0")

@Jasha10 Jasha10 added this to the Hydra 1.3.0 milestone Nov 26, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented Nov 27, 2022

After thinking about this some more, I'm not sure it make sense to support deleting items from ListConfig via the CLI.

Consider the following example:

python my_app.py ~config.list.0 ~config.list.1

Does the above delete items 0 and 1 from the list, or items 0 and 2?

In python, if you execute del lst[0]; del lst[1], then items 0 and 2 are deleted from the list. Meanwhile, if you execute del lst[1]; del lst[0] then items 0 and 1 are deleted. There is yet more complexity and order-dependence if we additionally consider appending items to a list (#1547).

I think that it would be nice if the mutations described by the command line overrides were applied to the ListConfig object in the same order that they are enumerated at the command line. I'm not sure if Hydra currently enforces such order dependence.

@omry
Copy link
Collaborator

omry commented Nov 28, 2022

I think the deletion logic in Hydra never considered lists.
It would probably not be too hard to add support for it.

@jbaczek
Copy link
Contributor Author

jbaczek commented Nov 28, 2022

Does the above delete items 0 and 1 from the list, or items 0 and 2?

Because you enter a command only a single time, then I think that all deletions should be parsed at the same time. Meaning that elements 0 and 1 should be deleted. This would alleviate the problem when you specify the same overrides but in different order.
I think, that it would be really confusing for me, when typing

python my_app.py ~config.list.0 ~config.list.1

would result in deleting items 0 and 2, but typing:

python my_app.py ~config.list.1 ~config.list.0

would result in deleting items 1 and 0.
From a peek into the code I know that keys are processed serially so this is not a minor change to the parsing algorithm. But this way it would make the most sense to me.

@Jasha10
Copy link
Collaborator

Jasha10 commented Dec 3, 2022

Btw one workaround is to use oc.dict.values to transform a yaml mapping into a list:

config:
    x: ${oc.dict.values:x_data}

x_data:
  a: elem_1
  b: elem_2
python my_app.py ~x_data.a  # similar effect to ~config.x.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants