Subclass init_args overwritten with multiple config files

[daniel-kubacki-str]

It seems that one configuration file completely overwrites the parameters of another config file in a subclass init_args when used with multiple config files. It works from the commandline, i.e. --trainer.init_args.<param> will overwrite the trainer <param>, but it doesn't work for multiple configuration files, i.e. a default one and one given on the command line.

I am using pytorch lightning Trainer as an example because it has a lot of arguments, not sure I would actually be subclassing it.

import json
import pytorch_lightning as pl
from jsonargparse import ArgumentParser, namespace_to_dict
from pprint import pprint

config1= {
    'trainer': {
        'class_path': 'pytorch_lightning.Trainer',
        'init_args': {
            'max_epochs': 101
        }
    }
}

config2 = {
    'trainer': {
        'class_path': 'pytorch_lightning.Trainer',
        'init_args': {
            'accelerator': 'ddp'
        }
    }
}

parser = ArgumentParser()
parser.add_argument('--cfg', action=ActionConfigFile)
parser.add_subclass_arguments(pl.Trainer, 'trainer')

args = parser.parse_args([
    '--cfg', json.dumps(config1),
    '--cfg', json.dumps(config2),
    '--trainer.init_args.limit_val_batches', '25',
])

I would expect to have

pprint(namespace_to_dict(args))
{'trainer': {
   ...
   'max_epochs': 101,
   'accelerator': 'ddp',
   'limit_val_batches': 25,
   ...
   }
}

But instead I get:

pprint(namespace_to_dict(args))
{'trainer': {
   ...
   'max_epochs': None,  #default
   'accelerator': 'ddp',
   'limit_val_batches': 25,
   ...
   }
}

[mauvilsa]

Actually this is the intended original behavior. Later the possibility of setting init args from individual command line arguments was added, in which the behavior made sense to be different. Although it has the intended behavior it does make sense to change it so that it is consistent with the individual command line arguments.

[daniel-kubacki-str]

The main use case is that I have a default config for a model or data module and then I provide a config with updates to the default config. At the moment this is not possible. Thanks.

[florianblume]

Has this issue been solved? I'm asking because that's the behavior I also need. I opened issue #113 but I think what I actually wanted to ask there was the question of this issue here, although I have a slight variation. I want to be able to update the init_args of a subclass, like so:

Default model config:

model;
    class_path: path.to.my.Model
    init_args:
        batch_size: 32
        img_size: [512, 512]
        ...

and the config I'm constantly editing to overwrite default values:

model:
    init_args:
        batch_size: 12

Now, I got the functionality as I would like it by modifying the update function in the Namespace class but this makes many tests fail. The code seems to be working though.

def update(self, value: Union['Namespace', Any], key: Optional[str] = None, only_unset: bool = False) -> 'Namespace':
        """Sets or replaces all items from the given nested namespace.

        Args:
            value: A namespace to update multiple values or other type to set in a single key.
            key: Branch key where to set the value. Required if value is not namepace.
            only_unset: Whether to only set the value if not set in namespace.
        """
        if not isinstance(value, Namespace):
            if not key:
                raise KeyError('Key is required if value not a Namespace.')
            if not only_unset or key not in self:
                self[key] = value
        else:
            prefix = key+'.' if key else ''
            for key, val in value.items():
                full_key = prefix + key
                if not only_unset or full_key not in self:
                    if isinstance(val, Namespace) or isinstance(val, dict):
                        if full_key in self:
                            if 'class_path' in val:
                                self[full_key]['class_path'] = val['class_path']
                            if 'init_args' in val:
                                if 'init_args' in self[full_key]:
                                    for init_args_key, init_args_val in val['init_args'].items():
                                        self[full_key]['init_args'].update(value=init_args_val,
                                                                           key=init_args_key)
                                else:
                                    self[full_key]['init_args'] = val['init_args']
                        else:
                            self[full_key] = val
                    else:
                        self[full_key] = val
        return self

This updates the existing config pretty dynamically, i.e. only updates the parts that are necessary and regardless of whether a key already exists. This means that the order of the configs I pass is also not important. Since the validity of the configuration is checked later anyways I wouldn't know why it's important to implement it in a way that the order is important (as mentioned in #85).

---

The problem now is that I have no clue how to make the tests run again. I even implemented tests myself for this "new" functionality because I didn't know there existed tests for this already. But the tests that check overwriting init_args run fine without my modification! Pytorch Lightning crashes nevertheless and doesn't allow me to only provide updated init_args. Any idea why this is the case?

[mauvilsa]

Has this issue been solved?

Not yet solved. Will try to look at this one next.

Now, I got the functionality as I would like it by modifying the update function in the Namespace class but this makes many tests fail.

argparse is not too well suited to extend so many things as jsonargparse does. Because of this many of the features are rather hacky and not trivial to understand. Some time ago the code related to subclasses (class_path and init_args) was much more spread around making it confusing. I have been moving things to a common place to be more in line with the principle of separation of concerns. Thus, this feature will not be a modification of the Namespace class. It will be probably a move of some code to typehints.py extending it to support the new feature. The idea will be that the same code is used for command line and config parsing so that the behavior is consistent between the two.

I wouldn't know why it's important to implement it in a way that the order is important (as mentioned in #85).

Parsing of command line arguments is done left to right. This is just how argparse works and can't be changed. Not really sure what you mean. The order is important, even for the feature proposed here.

I even implemented tests myself for this "new" functionality because I didn't know there existed tests for this already. But the tests that check overwriting init_args run fine without my modification!

Can you please share the tests that you wrote.

[florianblume]

Hey @mauvilsa,

thanks for your reply! I see, it's a lot more complicated than I anticipated ;) Maybe my code won't work with command line anymore but for now it does what I want so I'll stick with it temporarily.

Regarding the parsing direction: I just mean that if you first pass a config e.g. for model that contains only init_args and no class_path and then pass a class_path for model in on of the configs or even directly via command line in the subsequent parameters that could/should work as well, since we are first processing all the parameters and update the config and then check its integrity, right?

Regarding the tests: I added them to namespace_tests.py:

def test_update_only_init_args(self):
    ns1 = Namespace(model=Namespace(class_path='class', init_args=Namespace(a=1)))
    ns2 = Namespace(model=Namespace(init_args=Namespace(a=2)))
    ns1.update(ns2)
    self.assertEqual(ns1['model']['init_args']['a'], 2)
    self.assertEqual(ns1['model']['class_path'], 'class')

def test_update_only_class_path(self):
    ns1 = Namespace(model=Namespace(class_path='class', init_args=Namespace(a=1)))
    ns2 = Namespace(model=Namespace(class_path='class2'))
    ns1.update(ns2)
    self.assertEqual(ns1['model']['init_args']['a'], 1)
    self.assertEqual(ns1['model']['class_path'], 'class2')

def test_update_non_existing_class_path(self):
    ns1 = Namespace(model=Namespace(init_args=Namespace(a=1)))
    ns2 = Namespace(model=Namespace(class_path='class'))
    ns1.update(ns2)
    self.assertEqual(ns1['model']['init_args']['a'], 1)
    self.assertEqual(ns1['model']['class_path'], 'class')

def test_update_non_existing_init_args(self):
    ns1 = Namespace(model=Namespace(class_path='class'))
    ns2 = Namespace(model=Namespace(init_args=Namespace(a=1)))
    ns1.update(ns2)
    self.assertEqual(ns1['model']['init_args']['a'], 1)
    self.assertEqual(ns1['model']['class_path'], 'class')

def test_update_neither_existing_init_args_nor_class_path(self):
    ns1 = Namespace(model=Namespace())
    ns2 = Namespace(model=Namespace(class_path='class', init_args=Namespace(a=1)))
    ns1.update(ns2)
    self.assertEqual(ns1['model']['init_args']['a'], 1)
    self.assertEqual(ns1['model']['class_path'], 'class')

I only figured out that there were plenty of other related tests after implementing those, so bear with me if they are a bit funny ;)

[mauvilsa]

Regarding the parsing direction: I just mean that if you first pass a config e.g. for model that contains only init_args and no class_path and then pass a class_path for model in on of the configs or even directly via command line in the subsequent parameters that could/should work as well, since we are first processing all the parameters and update the config and then check its integrity, right?

The parsing of subclasses cannot be as simple as merging dicts. You are overlooking other jsonargparse features. There are several reason why the order is important. One reason is subconfigs. It is possible to give a config file which internally references another config. For example having a first command line argument --config=config1.yaml and inside that file there is model: model.yaml. If the model key isn't parsed in that position and a second argument specifies a dict with only init_args, then the merge would be between a string model.yaml and a dict. If the dict replaces the string, the parsing would be wrong because the contents of model.yaml haven't been taken into account yet. Furthermore, there could be nested subconfigs. Some model could have a parameter with a class type, take for instance backbone: torch.nn.Module. Inside model.yaml there could be the class_path and in the init_args a entry like backbone: lstm.yaml. Note that not only the contents of model.yaml need to be loaded in that position. It is also necessary to check that the class_path is valid, which parameters it has, and since one of the parameters is a subclass config, further parse that inner subconfig.

Another reason is that for a particular base class it is not guaranteed that all its subclasses have the same set of parameters. Suppose that in a first argument, a class_path and some init_args are given. Then in a second argument the class_path is changed to a class that does not accept one of the previously defined init_args. If the behavior were to just merge the init_args dicts, then that second class would receive an invalid parameter and the parsing would fail. This is why it was a design decision for the command line that when the class_path changes, the previous init_args are discarded.

[mauvilsa]

Has this issue been solved?

Indeed it seems this issue was already fixed with some other changes. I have added a unit test typehints_tests.py#L563-L591.

I looked at the possibility of having in a config init_args without a class_path and it is not a simple fix. For the time being I will close this issue and that other enhancement be tracked in issue #113.

[florianblume]

I see, thanks for the clarification. I wasn't aware of these additional features of jsonargparse and their implications. I guess for now I'll just stick to my small modification since that makes my workflow more failure proof (I tend to forget to modify the image size in the model configs for example).