diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e1bc219d..ca7e6e2a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -116,6 +116,18 @@ Fixed - ``add_class_arguments`` given a subscripted generic class, e.g. ``SomeClass[int]``, did not instantiate it, giving a ``Namespace`` instead of an instance (`#967 `__). +- A ``Union`` of a class type and an instance factory for it, e.g. + ``Union[Optimizer, Callable[[Iterable], Optimizer]]``, accepted no value at + all. The class subtype added as sub-default a placeholder for the parameter + that the factory receives when called, which made the value invalid for itself + and for the factory subtype (`#968 + `__). +- The ``shtab-bash`` completion script printed ``tput: command not found`` on + every completion in environments that don't have ``tput``, e.g. some + containers. Now the colors of the completion messages are resolved once when + the script is sourced and errors from ``tput`` are ignored, giving uncolored + messages when it is not available (`#968 + `__). Changed ^^^^^^^ diff --git a/jsonargparse/_completions.py b/jsonargparse/_completions.py index 3e1c9b67..2987b45b 100644 --- a/jsonargparse/_completions.py +++ b/jsonargparse/_completions.py @@ -275,6 +275,10 @@ def shtab_prepare_action(action, parser) -> None: # redraw-current-line, making readline itself redraw once the completion function returns. bash_compgen_typehint = """ [[ $- == *i* ]] && bind '"\\e[0n": redraw-current-line' 2>/dev/null +# tput gives the message colors when available, ignoring its errors, e.g. not installed or +# TERM unset, which leaves the colors empty +%(b)s="$(tput setaf 5 2>/dev/null || true)" +%(n)s="$(tput sgr0 2>/dev/null || true)" %(name)s() { local CHOICES="$1" WORD="$2" MESSAGE="$3" REQUIRE_PREFIX="$4" TOTAL="$5" local IFS=$'\\n' # choices may contain spaces, so split matches on newline only @@ -290,7 +294,7 @@ def shtab_prepare_action(action, parser) -> None: fi if [ ${#MATCH[@]} = 0 ]; then if [ "$COMP_TYPE" = 63 ]; then - printf "%(b)s\\n%%s%%s\\n%(n)s" "$MESSAGE" "$MATCHED" >&2 + printf "${%(b)s}\\n%%s%%s\\n${%(n)s}" "$MESSAGE" "$MATCHED" >&2 printf '\\033[5n' >&2 fi else @@ -298,14 +302,14 @@ def shtab_prepare_action(action, parser) -> None: echo "$match" done if [ "$COMP_TYPE" = 63 ]; then - printf "%(b)s\\n%%s%%s%(n)s" "$MESSAGE" "$MATCHED" >&2 + printf "${%(b)s}\\n%%s%%s${%(n)s}" "$MESSAGE" "$MATCHED" >&2 fi fi } """ % { "name": bash_compgen_typehint_name, - "b": "$(tput setaf 5)", - "n": "$(tput sgr0)", + "b": "_jsonargparse_{prog}_color_message", + "n": "_jsonargparse_{prog}_color_reset", } diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index ed278974..80831bc3 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -1507,13 +1507,17 @@ def adapt_typehints( sorted_subtypes = sort_subtypes_for_union(subtypehints, val, prev_val, append) for subtype in sorted_subtypes: try: - vals.append(adapt_typehints(val, subtype, **adapt_kwargs)) - break + # a pristine value, since adapting can modify it in place + subtype_val = adapt_typehints(pristine_value(val), subtype, **adapt_kwargs) except Exception as ex: if subtype is str and not isinstance(val, str) and isinstance(orig_val, str): vals.append(orig_val) continue vals.append(ex) + continue + vals.append(subtype_val) + if not sub_defaults_invalidate_value(subtype_val, subtype, sorted_subtypes, adapt_kwargs): + break if all(isinstance(v, Exception) for v in vals): raise_union_unexpected_value(sorted_subtypes, val, vals) val = next((v for v in reversed(vals) if not isinstance(v, Exception))) @@ -2798,6 +2802,33 @@ def rebuild_typehint_args(typehint, new_args): return typehint +def pristine_value(val): + """Copy of a value, so that adapting it against a union subtype does not affect the others.""" + return val.clone() if isinstance(val, Namespace) else val + + +def sub_defaults_invalidate_value(value, subtype, subtypes, adapt_kwargs) -> bool: + """Whether the sub-defaults that a union subtype added make the value invalid for it. + + Sub-defaults are added leniently, so a class subtype can add a placeholder for a required + parameter, e.g. the object that an instance factory receives when called. The placeholder + then invalidates the value, both for the subtype that added it and for the subtypes that + would have accepted the value without it, see sub_defaults_context. + """ + if not ( + sub_defaults.get() + and is_subclass_spec(value) + and any(ActionTypeHint.is_return_subclass_typehint(s) for s in subtypes) + ): + return False + with parser_context(lenient_check=False): + try: + adapt_typehints(value.clone(), subtype, **adapt_kwargs) + except Exception: + return True + return False + + def sort_subtypes_for_union(subtypes, val, prev_val, append): """Sorts the subtypes of a union for the parsing of a given value. diff --git a/jsonargparse_tests/test_shtab.py b/jsonargparse_tests/test_shtab.py index 7f5ccd6f..aa99b6ca 100644 --- a/jsonargparse_tests/test_shtab.py +++ b/jsonargparse_tests/test_shtab.py @@ -345,6 +345,38 @@ def test_bash_script_binds_redraw_current_line(parser): assert "bind '\"\\e[0n\": redraw-current-line'" in shtab_script +def run_bash_typehint_completion(shtab_script, tmp_path, dest, word="", prefix=""): + shtab_script_path = tmp_path / "comp.sh" + shtab_script_path.write_text(shtab_script) + sh = f'{prefix}source {shtab_script_path}; COMP_TYPE=63 _jsonargparse_tool_{dest}_typehint "{word}"' + popen = subprocess.Popen(["bash", "-c", sh], stdout=subprocess.PIPE, stderr=subprocess.PIPE) + out, err = popen.communicate() + return out.decode(), err.decode() + + +def get_bash_tput_color(): + out = subprocess.run(["bash", "-c", "tput setaf 5 2>/dev/null"], capture_output=True) + return out.stdout.decode() + + +def test_bash_message_colored_when_tput_available(parser, tmp_path): + color = get_bash_tput_color() + if not color: + pytest.skip("tput command not available") # pragma: no cover + parser.add_argument("--num", type=int) + _, err = run_bash_typehint_completion(get_shtab_script(parser, "bash"), tmp_path, "num") + assert f"{color}\nExpected type: int" in err + + +def test_bash_message_uncolored_when_tput_not_available(parser, tmp_path): + parser.add_argument("--num", type=int) + shtab_script = get_shtab_script(parser, "bash") + _, err = run_bash_typehint_completion(shtab_script, tmp_path, "num", prefix='PATH=""; ') + assert "\nExpected type: int" in err + assert "tput" not in err + assert "not found" not in err + + def get_bash_major_version(): out = subprocess.run(["bash", "-c", 'echo "${BASH_VERSINFO[0]}"'], capture_output=True) try: diff --git a/jsonargparse_tests/test_typehints.py b/jsonargparse_tests/test_typehints.py index 8fc05fe3..34b7eb8a 100644 --- a/jsonargparse_tests/test_typehints.py +++ b/jsonargparse_tests/test_typehints.py @@ -3061,6 +3061,31 @@ def test_callable_args_return_type_class(parser, subtests): assert "--optimizer.params" not in help_str +optimizer_or_factory = Optional[Union[Optimizer, Callable[[List[float]], Optimizer]]] + + +def test_callable_args_return_type_class_in_union_factory(parser): + parser.add_argument("--optimizer", type=optimizer_or_factory) + cfg = parser.parse_args(["--optimizer=Adam", "--optimizer.lr=0.01"]) + assert cfg.optimizer.class_path == f"{__name__}.Adam" + assert cfg.optimizer.init_args == Namespace(lr=0.01, momentum=0.0) + init = parser.instantiate(cfg) + optimizer = init.optimizer([4.5, 6.7]) + assert isinstance(optimizer, Adam) + assert optimizer.params == [4.5, 6.7] + assert optimizer.lr == 0.01 + + +def test_callable_args_return_type_class_in_union_instance(parser): + parser.add_argument("--optimizer", type=optimizer_or_factory) + value = {"class_path": "Adam", "init_args": {"params": [4.5, 6.7], "lr": 0.02}} + cfg = parser.parse_args([f"--optimizer={json.dumps(value)}"]) + init = parser.instantiate(cfg) + assert isinstance(init.optimizer, Adam) + assert init.optimizer.params == [4.5, 6.7] + assert init.optimizer.lr == 0.02 + + class OptimizerFactory(Protocol): def __call__(self, params: List[float]) -> Optimizer: ...