Boolean config item always give 'False' value.

[idkwid2022]

Describe the bug

Boolean config items always give 'False' value even when set 'True' or 'true' in config.yaml.

Please see function setup_parser() on utils.py, on line 29. I think it's missing default value, like on line 31, or may be just skip checking the boolean type, and treat it like another?

code-generator/src/templates/template-vision-segmentation/utils.py

Lines 21 to 33 in f137219

	def setup_parser():
	with open("config.yaml", "r") as f:
	config = yaml.safe_load(f.read())
	parser = ArgumentParser()
	parser.add_argument("--backend", default=None, type=str)
	for k, v in config.items():
	if isinstance(v, bool):
	parser.add_argument(f"--{k}", action="store_true")
	else:
	parser.add_argument(f"--{k}", default=v, type=type(v))
	return parser

Expected result

Boolean config items give value as set.

Steps to reproduce

Set any boolean config item in config.yaml to 'true'.

[vfdev-5]

Thanks for reporting @idkwid2022 !
I have to check details. As far as I know for arg parser to pass boolean flag is done with "store_true" action as it should not work correctly if we're using type and default args as for other types. Maybe the issue is elsewhere...

[idkwid2022]

I think it's because how it's used in this project.

My understanding is function setup_parser is correct as is, if it's intended only to setup the parser, but because on main.py you did not pass any value at all:

code-generator/src/templates/template-vision-segmentation/main.py

Lines 197 to 220 in f137219

	def main():
	config = setup_parser().parse_args()
	#::: if (it.dist === 'spawn') { :::#
	#::: if (it.nproc_per_node && it.nnodes > 1 && it.master_addr && it.master_port) { :::#
	kwargs = {
	"nproc_per_node": config.nproc_per_node,
	"nnodes": config.nnodes,
	"node_rank": config.node_rank,
	"master_addr": config.master_addr,
	"master_port": config.master_port,
	}
	#::: } else if (it.nproc_per_node) { :::#
	kwargs = {"nproc_per_node": config.nproc_per_node}
	#::: } :::#
	with idist.Parallel(config.backend, **kwargs) as p:
	p.run(run, config=config)
	#::: } else { :::#
	with idist.Parallel(config.backend) as p:
	p.run(run, config=config)
	#::: } :::#
	if __name__ == "__main__":
	main()

When we run python main.py, the parser will not have any argument value at all, and action="store_true" will give False as default value.

But if the intended purpose was to just use config.yaml, it's incorrect.

I modify mine to skip the type checking:

def setup_parser():
    with open("config.yaml", "r") as f:
        config = yaml.safe_load(f.read())

    parser = ArgumentParser()
    parser.add_argument("--backend", default=None, type=str)
    for k, v in config.items():
        parser.add_argument(f"--{k}", default=v, type=type(v))

    return parser

[vfdev-5]

I see your point. I tried your solution and default one. Actually, the point is the following:
Currently, config.yaml defines config variables and their default values and for boolean as you stated no matter the value is in config.yaml, config object will have False. To make it as True on execution we need to call:

python main.py --debug --use_amp

so, that config object will have config.debug == True and config.use_amp == True.
If we take the change you proposed in your comment, then python main.py --debug wont work anymore. And further more it lead to the following issue: python main.py --debug=False -> config.debug == True
On the other hand I agree with you that we should be able to override config.yaml boolean vars and set default value as True.

@ydcjeff can you suggest what we should do here ?

[vfdev-5]

Maybe, we can update setup_parser() as following:

def setup_parser():
    with open("config.yaml", "r") as f:
        config = yaml.safe_load(f.read())

    parser = ArgumentParser()
    parser.add_argument("--backend", default=None, type=str)
    for k, v in config.items():
        if isinstance(v, bool):
            prefix = "" if not v else "no_"
            parser.add_argument(
                f"--{prefix}{k}", dest=f"{k}", action=f"store_{str(not v).lower()}"
            )
        else:
            parser.add_argument(f"--{k}", default=v, type=type(v))

    return parser

So, it will behave as following with config.yaml:

debug: false
use_amp: true

1. Default values:

python main.py
> config.debug==False, config.use_amp==True

2. Override debug to True

python main.py --debug
> config.debug==True, config.use_amp==True

3. Override use_amp to False

python main.py --no_use_amp
> config.debug==False, config.use_amp==False

@idkwid2022 what do you think ?

[idkwid2022]

I think it's good, but if for ignorance guy like me, I think it should be mentioned in the README.md.

I got this problem because I read

code-generator/src/templates/template-vision-segmentation/README.md

Line 114 in f137219

python main.py

and immediately assumed everything will be set with config.yaml, the other lines (backend, nproc_per_node, etc.) didn't show because I didn't use distributed training. And I was ignorance enough to not think about setting arguments with command line. And it worked with other items like train_batch_size, data-path, etc.

[vfdev-5]

Yes, you are right, we should have mentioned that in Readme !

Would you like to send a PR with the fixes: 1) for setup_parser and 2) for Readme ?

[idkwid2022]

I'm gonna have to leave it here for now. I'm already overwhelmed with so much things in image segmentation topic. I think it's good enough for now that the issue is raised and we already have your solution here.

So, I think this can be closed.

[theory-in-progress]

With the new config system in the PR #243

code-generator/src/templates/template-common/utils.py

Lines 44 to 74 in 9393a04

	def get_default_parser():
	parser = ArgumentParser()
	parser.add_argument("config", type=Path, help="Config file path")
	parser.add_argument(
	"--backend",
	default=None,
	choices=["nccl", "gloo"],
	type=str,
	help="DDP backend",
	)
	return parser
	def setup_config(parser=None):
	if parser is None:
	parser = get_default_parser()
	args = parser.parse_args()
	config_path = args.config
	with open(config_path, "r") as f:
	config = yaml.safe_load(f.read())
	optional_attributes = ["train_epoch_length", "eval_epoch_length"]
	for attr in optional_attributes:
	config[attr] = config.get(attr, None)
	for k, v in config.items():
	setattr(args, k, v)
	return args

The attributes are set accordingly. I don't think the problem exists now.

Now we can just set bool to the parameters in the config file and it will work.