Skip to content

Commit

Permalink
Deprecate action=ActionJsonnetExtVars in favor of type=dict.
Browse files Browse the repository at this point in the history
  • Loading branch information
mauvilsa committed Aug 22, 2023
1 parent 16aa1f7 commit 06e1b9b
Show file tree
Hide file tree
Showing 9 changed files with 125 additions and 45 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ Added
- Resolve types that use ``TYPE_CHECKING`` blocks (`#337 comment
<https://github.com/omni-us/jsonargparse/issues/337#issuecomment-1665055459>`__).
- Improved resolving of nested forward references in types.
- The ``ext_vars`` for an ``ActionJsonnet`` argument can now have a default.

Deprecated
^^^^^^^^^^
- ``ActionJsonnetExtVars`` is deprecated and will be removed in v5.0.0. Instead
use ``type=dict``.


v4.23.1 (2023-08-04)
Expand Down
4 changes: 2 additions & 2 deletions DOCUMENTATION.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2288,10 +2288,10 @@ dictionary of variables. Its use would be as follows:

.. testcode:: jsonnet

from jsonargparse import ArgumentParser, ActionJsonnet, ActionJsonnetExtVars
from jsonargparse import ArgumentParser, ActionJsonnet

parser = ArgumentParser()
parser.add_argument("--in_ext_vars", action=ActionJsonnetExtVars())
parser.add_argument("--in_ext_vars", type=dict)
parser.add_argument("--in_jsonnet", action=ActionJsonnet(ext_vars="in_ext_vars"))

For example, if a jsonnet file required some external variable ``param``, then
Expand Down
18 changes: 18 additions & 0 deletions jsonargparse/_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,16 @@ def __call__(self, parser, cfg, values, option_string=None):
def set_default_error():
raise ValueError("ActionConfigFile does not accept a default, use default_config_files.")

@staticmethod
def _ensure_single_config_argument(container, action):
if is_subclass(action, ActionConfigFile) and any(isinstance(a, ActionConfigFile) for a in container._actions):
raise ValueError("A parser is only allowed to have a single ActionConfigFile argument.")

@staticmethod
def _add_print_config_argument(container, action):
if isinstance(action, ActionConfigFile) and getattr(container, "_print_config", None) is not None:
container.add_argument(container._print_config, action=_ActionPrintConfig)

@staticmethod
def apply_config(parser, cfg, dest, value) -> None:
from ._link_arguments import skip_apply_links
Expand Down Expand Up @@ -491,6 +501,14 @@ def __init__(
if not isinstance(self._parser, import_object("jsonargparse.ArgumentParser")):
raise ValueError("Expected parser keyword argument to be an ArgumentParser.")

@staticmethod
def _is_valid_action_parser(parser, action) -> bool:
if not isinstance(action, ActionParser):
return False
if action._parser == parser:
raise ValueError("Parser cannot be added as a subparser of itself.")
return True

@staticmethod
def _move_parser_actions(parser, args, kwargs):
subparser = kwargs.pop("action")._parser
Expand Down
25 changes: 9 additions & 16 deletions jsonargparse/_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import inspect
import logging
import os
import re
import sys
from contextlib import suppress
from typing import (
Expand Down Expand Up @@ -38,10 +37,10 @@
parent_parsers,
previous_config,
)
from ._common import is_dataclass_like, is_subclass, lenient_check, parser_context
from ._common import is_dataclass_like, lenient_check, parser_context
from ._deprecated import ParserDeprecations
from ._formatters import DefaultHelpFormatter, empty_help, get_env_var
from ._jsonnet import ActionJsonnet, ActionJsonnetExtVars
from ._jsonnet import ActionJsonnet
from ._jsonschema import ActionJsonSchema
from ._link_arguments import ActionLink, ArgumentLinking
from ._loaders_dumpers import (
Expand Down Expand Up @@ -107,19 +106,13 @@ def add_argument(self, *args, enable_path: bool = False, **kwargs):
"""
parser = self.parser if hasattr(self, "parser") else self
if "action" in kwargs:
if isinstance(kwargs["action"], ActionParser):
if kwargs["action"]._parser == parser:
raise ValueError("Parser cannot be added as a subparser of itself.")
if ActionParser._is_valid_action_parser(parser, kwargs["action"]):
return ActionParser._move_parser_actions(parser, args, kwargs)
if is_subclass(kwargs["action"], ActionConfigFile) and any(
isinstance(a, ActionConfigFile) for a in self._actions
):
raise ValueError("A parser is only allowed to have a single ActionConfigFile argument.")
ActionConfigFile._ensure_single_config_argument(self, kwargs["action"])
if "type" in kwargs:
if is_dataclass_like(kwargs["type"]):
theclass = kwargs.pop("type")
nested_key = re.sub("^--", "", args[0])
self.add_dataclass_arguments(theclass, nested_key, **kwargs)
nested_key = args[0].lstrip("-")
self.add_dataclass_arguments(kwargs.pop("type"), nested_key, **kwargs)
return _find_action(parser, nested_key)
if ActionTypeHint.is_supported_typehint(kwargs["type"]):
args = ActionTypeHint.prepare_add_argument(
Expand All @@ -131,8 +124,8 @@ def add_argument(self, *args, enable_path: bool = False, **kwargs):
)
action = super().add_argument(*args, **kwargs)
action.logger = self._logger # type: ignore
if isinstance(action, ActionConfigFile) and getattr(self, "_print_config", None) is not None:
self.add_argument(self._print_config, action=_ActionPrintConfig) # type: ignore
ActionConfigFile._add_print_config_argument(self, action)
ActionJsonnet._check_ext_vars_action(parser, action)
if is_meta_key(action.dest):
raise ValueError(f'Argument with destination name "{action.dest}" not allowed.')
if action.help is None:
Expand Down Expand Up @@ -1252,7 +1245,7 @@ def _apply_actions(
if isinstance(action, _ActionConfigLoad):
config_keys.add(action_dest)
keys.append(action_dest)
elif isinstance(action, ActionJsonnetExtVars):
elif getattr(action, "jsonnet_ext_vars", False):
prev_cfg[action_dest] = value
cfg[action_dest] = value
return cfg[parent_key] if parent_key else cfg
Expand Down
18 changes: 18 additions & 0 deletions jsonargparse/_deprecated.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

__all__ = [
"ActionEnum",
"ActionJsonnetExtVars",
"ActionOperators",
"ActionPath",
"ActionPathList",
Expand Down Expand Up @@ -581,3 +582,20 @@ def py36_set_deprecated_module_attributes(globs):
"import_docstring_parse": ("_optionals", "import_docstring_parser"),
},
)


@deprecated(
"""
ActionJsonnetExtVars was deprecated in v4.24.0 and will be removed in
v5.0.0. Instead use ``type=dict``.
"""
)
class ActionJsonnetExtVars:
"""Action to add argument to provide ext_vars for jsonnet parsing."""

def __call__(self, *args, **kwargs):
from ._typehints import ActionTypeHint

action = ActionTypeHint(typehint=dict)(**kwargs)
action.jsonnet_ext_vars = True
return action
46 changes: 24 additions & 22 deletions jsonargparse/_jsonnet.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from typing import Any, Dict, Optional, Tuple, Union

from ._actions import Action, _is_action_value_list
from ._actions import Action, _find_action, _is_action_value_list
from ._common import parser_context
from ._jsonschema import ActionJsonSchema
from ._loaders_dumpers import get_loader_exceptions, load_value
Expand All @@ -12,28 +12,10 @@
import_jsonnet,
import_jsonschema,
)
from ._typehints import ActionTypeHint
from ._util import Path, argument_error

__all__ = [
"ActionJsonnetExtVars",
"ActionJsonnet",
]


class ActionJsonnetExtVars(ActionJsonSchema):
"""Action to add argument to provide ext_vars for jsonnet parsing."""

def __init__(self, **kwargs):
"""Initializer for ActionJsonnetExtVars instance."""
if kwargs:
super().__init__(**kwargs)
else:
super().__init__(schema={"type": "object"}, with_meta=False)

def __call__(self, *args, **kwargs):
if len(args) == 0 and "default" not in kwargs:
kwargs["default"] = {}
return super().__call__(*args, _class_type=ActionJsonnetExtVars, **kwargs)
__all__ = ["ActionJsonnet"]


class ActionJsonnet(Action):
Expand Down Expand Up @@ -67,7 +49,7 @@ def __init__(
try:
schema = load_value(schema)
except get_loader_exceptions() as ex:
raise ValueError(f"Problems parsing schema :: {ex}") from ex
raise ValueError(f"Problems parsing schema: {ex}") from ex
jsonvalidator.check_schema(schema)
self._validator = ActionJsonSchema._extend_jsonvalidator_with_default(jsonvalidator)(schema)
else:
Expand Down Expand Up @@ -113,6 +95,26 @@ def _check_type(self, value, cfg):
raise TypeError(f'Parser key "{self.dest}"{elem}: {ex}') from ex
return value if islist else value[0]

@staticmethod
def _check_ext_vars_action(parser, action):
if isinstance(action, ActionJsonnet) and action._ext_vars:
ext_vars_action = _find_action(parser, action._ext_vars)
if not ext_vars_action:
raise ValueError(f"No argument found for ext_vars='{action._ext_vars}'")
ext_vars_type = isinstance(ext_vars_action, ActionTypeHint) and ext_vars_action._typehint
if ext_vars_type not in {dict, Dict}:
raise ValueError(
f"Type for ext_vars='{action._ext_vars}' argument must be dict, given: {ext_vars_type}"
)
if ext_vars_action.default is None:
ext_vars_action.default = {}
if not isinstance(ext_vars_action.default, dict):
raise ValueError(
f"Default value for the ext_vars='{action._ext_vars}' argument "
f"must be dict or None, given: {ext_vars_action.default}"
)
ext_vars_action.jsonnet_ext_vars = True

@staticmethod
def split_ext_vars(ext_vars: Optional[Dict[str, Any]]) -> Tuple[Dict[str, Any], Dict[str, Any]]:
"""Splits an ext_vars dict into the ext_codes and ext_vars required by jsonnet.
Expand Down
19 changes: 18 additions & 1 deletion jsonargparse_tests/test_deprecated.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from jsonargparse import (
CLI,
ActionConfigFile,
ActionJsonnet,
ArgumentError,
ArgumentParser,
Path,
Expand All @@ -23,6 +24,7 @@
)
from jsonargparse._deprecated import (
ActionEnum,
ActionJsonnetExtVars,
ActionOperators,
ActionPath,
ActionPathList,
Expand All @@ -31,13 +33,14 @@
shown_deprecation_warnings,
usage_and_exit_error_handler,
)
from jsonargparse._optionals import url_support
from jsonargparse._optionals import jsonnet_support, url_support
from jsonargparse._util import LoggerProperty, argument_error
from jsonargparse_tests.conftest import (
get_parser_help,
skip_if_docstring_parser_unavailable,
skip_if_requests_unavailable,
)
from jsonargparse_tests.test_jsonnet import example_2_jsonnet


@pytest.fixture(autouse=True)
Expand Down Expand Up @@ -475,3 +478,17 @@ def test_import_from_module(module, attr):
getattr(module, attr)
if sys.version_info[:2] != (3, 6):
assert "Only use the public API" in str(w[-1].message)


@pytest.mark.skipif(not jsonnet_support, reason="jsonnet package is required")
def test_action_jsonnet_ext_vars(parser):
with catch_warnings(record=True) as w:
parser.add_argument("--ext_vars", action=ActionJsonnetExtVars())
assert "ActionJsonnetExtVars was deprecated" in str(w[-1].message)
parser.add_argument("--jsonnet", action=ActionJsonnet(ext_vars="ext_vars"))

cfg = parser.parse_args(["--ext_vars", '{"param": 123}', "--jsonnet", example_2_jsonnet])
assert 123 == cfg.jsonnet["param"]
assert 9 == len(cfg.jsonnet["records"])
assert "#8" == cfg.jsonnet["records"][-2]["ref"]
assert 15.5 == cfg.jsonnet["records"][-2]["val"]
32 changes: 29 additions & 3 deletions jsonargparse_tests/test_jsonnet.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
from jsonargparse import (
ActionConfigFile,
ActionJsonnet,
ActionJsonnetExtVars,
ActionJsonSchema,
ArgumentError,
ArgumentParser,
Expand Down Expand Up @@ -135,7 +134,7 @@ def __init__(self, name: str = "Lucky", prize: int = 100):

@skip_if_jsonschema_unavailable
def test_action_jsonnet(parser):
parser.add_argument("--input.ext_vars", action=ActionJsonnetExtVars())
parser.add_argument("--input.ext_vars", type=dict)
parser.add_argument(
"--input.jsonnet",
action=ActionJsonnet(ext_vars="input.ext_vars", schema=json.dumps(example_schema)),
Expand All @@ -157,7 +156,7 @@ def test_action_jsonnet(parser):


def test_action_jsonnet_save_config_metadata(parser, tmp_path):
parser.add_argument("--ext_vars", action=ActionJsonnetExtVars())
parser.add_argument("--ext_vars", type=dict)
parser.add_argument("--jsonnet", action=ActionJsonnet(ext_vars="ext_vars"))
parser.add_argument("--cfg", action=ActionConfigFile)

Expand Down Expand Up @@ -221,6 +220,33 @@ def test_action_jsonnet_parse_method():
assert 15.5 == parsed["records"][-2]["val"]


def test_action_jsonnet_ext_vars_default(parser):
parser.add_argument("--ext_vars", type=dict, default={"param": 432})
parser.add_argument("--jsonnet", action=ActionJsonnet(ext_vars="ext_vars"))
cfg = parser.parse_args(["--jsonnet", example_2_jsonnet])
assert 432 == cfg.jsonnet["param"]


def test_action_jsonnet_ext_vars_not_defined(parser):
with pytest.raises(ValueError) as ctx:
parser.add_argument("--jsonnet", action=ActionJsonnet(ext_vars="ext_vars"))
ctx.match("No argument found for ext_vars")


def test_action_jsonnet_ext_vars_invalid_type(parser):
parser.add_argument("--ext_vars", type=list)
with pytest.raises(ValueError) as ctx:
parser.add_argument("--jsonnet", action=ActionJsonnet(ext_vars="ext_vars"))
ctx.match("Type for ext_vars='ext_vars' argument must be dict")


def test_action_jsonnet_ext_vars_invalid_default(parser):
parser.add_argument("--ext_vars", type=dict, default="none")
with pytest.raises(ValueError) as ctx:
parser.add_argument("--jsonnet", action=ActionJsonnet(ext_vars="ext_vars"))
ctx.match("Default value for the ext_vars='ext_vars' argument must be dict or None")


# other tests


Expand Down
2 changes: 1 addition & 1 deletion jsonargparse_tests/test_jsonschema.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def parser_schema_object(parser):
},
"additionalProperties": False,
}
parser.add_argument("--op2", action=ActionJsonSchema(schema=schema_object))
parser.add_argument("--op2", action=ActionJsonSchema(schema=schema_object, with_meta=False))
parser.add_argument("--cfg", action=ActionConfigFile)
return parser

Expand Down

0 comments on commit 06e1b9b

Please sign in to comment.