ConfigArgParse icon indicating copy to clipboard operation
ConfigArgParse copied to clipboard

Defaults override boolean values provided in config file.

Open hillierdani opened this issue 5 years ago • 5 comments

Hi, The following self-contained code demonstrates, that when parsing booleans, defaults override, whereas the same does not happen with int:

import pytest
import tempfile
import configargparse

@pytest.fixture
def example_config_file():
    cfg_str = '''correlation_thr = 0.3
basedir = /mnt/datafast
plot_curves = True
experiment_names = [RD1005, RD1006a, RD1007, RD1008, RD1009]
use_saved = 1
area_thr = 150
power_thr = 0.2
gene = TRP
recalculate_quantified = 1000
analyze = duration_s
include_all_days = False
take_best_days = 0
required_repetitions = 1'''
    with tempfile.NamedTemporaryFile('w+') as tf:
        tf.write(cfg_str)
        tf.seek(0)
        tf.flush()
        yield tf

def test_config_file(example_config_file):
    '''
    Tests whether values written to config file will get parsed correctly.
    Make sure you added --noconftest to "Additional arguments" in pycharm configuration or on command-line, so that confstest.py will not indirectly invoke visexpA.configuration.
    '''
    import sys
    parser = configargparse.get_arg_parser()
    parser.add_argument('--param_file', default = example_config_file.name, is_config_file=True)  # default value for temp file
    parser.add_argument('--area_thr', type=int, default=100)
    parser.add_argument('--take_best_days', default=1, type=int)
    parser.add_argument('--include_all_days', default=1, type=bool)
    parsed, unrecognized = parser.parse_known_args(config_file_contents=example_config_file.read())
    assert parsed.area_thr == 150
    assert parsed.take_best_days == False
    assert parsed.include_all_days == False
    parsed1, unrecognized1 = parser.parse_known_args()  # load via the provided default filename
    assert parsed1.take_best_days == False

Giving:

example_config_file = <tempfile._TemporaryFileWrapper object at 0x7fe218b7e4e0>

    def test_config_file(example_config_file):
        '''
        Tests whether values written to config file will get parsed correctly.
        Make sure you added --noconftest to "Additional arguments" in pycharm configuration or on command-line, so that confstest.py will not indirectly invoke visexpA.configuration.
        '''
        import sys
        parser = configargparse.get_arg_parser()
        parser.add_argument('--param_file', default = example_config_file.name, is_config_file=True)  # default value for temp file
        parser.add_argument('--area_thr', type=int, default=100)
        parser.add_argument('--take_best_days', default=1, type=int)
        parser.add_argument('--include_all_days', default=1, type=bool)
        parsed, unrecognized = parser.parse_known_args(config_file_contents=example_config_file.read())
        assert parsed.area_thr == 150
        assert parsed.take_best_days == False
>       assert parsed.include_all_days == False
E       assert True == False

test_configuration.py:40: AssertionError

The only difference between take_best_days and include_all_days is that the latter is defined as bool. I would be grateful for a clarification on why this happens and whether this is the desired behavior. Thanks.

hillierdani avatar Feb 12 '20 15:02 hillierdani

This looks like a bug. I don't immediately see why this is happening, but a PR to fix it would be appreciated.

bw2 avatar Apr 02 '20 08:04 bw2

I've looked into this and it seems that this issue is related to argparse and intended. See This python issue on the matter.

The TLDR is: In add_argument the parameter 'type=bool' for "include_all_days" will result in calling 'bool("False")' for "include_all_days". This call returns "True" as only 'bool("")' results False.

But there seems to be another bug related to argparse. Consider the following example:

config.yaml: include_all_days: ""

test.py

import configargparse
parser = configargparse.get_arg_parser()
parser.add_argument('--config', default = 'config.yaml', is_config_file=True) 
parser.add_argument('--include_all_days', default=1, type=bool)
parsed = parser.parse_args()
print(vars(parsed))

will result in: {'config': 'config.txt', 'include_all_days': True}

This seems to be a bug(?) from configargparse.py, line 613: namespace, unknown_args = argparse.ArgumentParser.parse_known_args( self, args=args, namespace=namespace) where variable args=["include_all_days=''"}. Which will result in a call of bool(' " " ') (without spaces), again resulting in True. This may be the bug in argparse. Easiest way to resolve this would be to strip ' and " when empty. This should result in a call with args=["include_all_days="] which should evaluate into the right value.

Ghaarg avatar Apr 16 '20 16:04 Ghaarg

I asked if this behaviour is expected at pythons bugtracker. You can look at the answers here.

Ghaarg avatar Apr 17 '20 08:04 Ghaarg

With the answers from the issue above i have a solution to the problem, albeit it is a bit counter intuitive. You can use 'store_false' to reach the goal of storing false with default true:

config.yaml:

include_all_days
include_no_days: true

test.py ´´´ import configargparse parser = configargparse.get_arg_parser() parser.add_argument('--config', default = 'config.yaml', is_config_file=True) parser.add_argument('--include_all_days', default=True, action="store_const", const=False) parser.add_argument('--include_no_days', default=True, action="store_false") parsed = parser.parse_args() print(vars(parsed)) ´´´

Both actions "store_const" and "store_false" work the same and both yaml entries do the same. But it is a bit counter intuitive to store "true" in the yaml to set a variable to false for the argparser.

Ghaarg avatar Apr 17 '20 08:04 Ghaarg

Thanks very much for going after this issue, I will check whether this solves my problem and post my test case here soon.

hillierdani avatar Apr 17 '20 08:04 hillierdani