Repository: reframe-hpc/reframe

Issue:
Make variables and parameters `loggable` by default
I think that variables and parameters should be loggable by default, since they usually represent defining attributes of the test being run and normally users would like to see them by default in the perflogs.

Discussion:

Patch:
diff --git a/reframe/core/parameters.py b/reframe/core/parameters.py
index c8659589..195cc21f 100644
--- a/reframe/core/parameters.py
+++ b/reframe/core/parameters.py
@@ -121,7 +121,8 @@ class TestParam:
 
     :param loggable: Mark this parameter as loggable. If :obj:`True`, this
         parameter will become a log record attribute under the name
-        ``check_NAME``, where ``NAME`` is the name of the parameter.
+        ``check_NAME``, where ``NAME`` is the name of the parameter (default:
+        :obj:`True`)
 
     :returns: A new test parameter.
 
@@ -130,10 +131,14 @@ class TestParam:
 
     .. versionadded:: 3.11.0
        The ``loggable`` argument is added.
+
+    .. versionchanged:: 4.5
+       Parameters are now loggable by default.
+
     '''
 
     def __init__(self, values=None, inherit_params=False,
-                 filter_params=None, fmt=None, loggable=False):
+                 filter_params=None, fmt=None, loggable=True):
         if values is None:
             values = []
 
diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py
index a6f51ac6..db53c4f2 100644
--- a/reframe/core/pipeline.py
+++ b/reframe/core/pipeline.py
@@ -236,8 +236,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: .. versionchanged:: 3.11.0
     #:    Extend syntax to support features and key/value pairs.
-    valid_prog_environs = variable(typ.List[typ.Str[_VALID_ENV_SYNTAX]],
-                                   loggable=True)
+    valid_prog_environs = variable(typ.List[typ.Str[_VALID_ENV_SYNTAX]])
 
     #: List of systems or system features or system properties required by this
     #: test.
@@ -300,8 +299,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #:  .. versionchanged:: 3.11.0
     #:     Extend syntax to support features and key/value pairs.
-    valid_systems = variable(typ.List[typ.Str[_VALID_SYS_SYNTAX]],
-                             loggable=True)
+    valid_systems = variable(typ.List[typ.Str[_VALID_SYS_SYNTAX]])
 
     #: A detailed description of the test.
     #:
@@ -328,7 +326,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`str`
     #: :default: ``''``
-    sourcepath = variable(str, value='', loggable=True)
+    sourcepath = variable(str, value='')
 
     #: The directory containing the test's resources.
     #:
@@ -358,7 +356,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:     .. versionchanged:: 3.0
     #:        Default value is now conditionally set to either ``'src'`` or
     #:        :class:`None`.
-    sourcesdir = variable(str, type(None), value='src', loggable=True)
+    sourcesdir = variable(str, type(None), value='src')
 
     #: .. versionadded:: 2.14
     #:
@@ -375,7 +373,8 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`str` or :class:`reframe.core.buildsystems.BuildSystem`.
     #: :default: :class:`None`.
-    build_system = variable(type(None), field=BuildSystemField, value=None)
+    build_system = variable(type(None), field=BuildSystemField, value=None,
+                            loggable=False)
 
     #: .. versionadded:: 3.0
     #:
@@ -387,7 +386,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`List[str]`
     #: :default: ``[]``
-    prebuild_cmds = variable(typ.List[str], value=[], loggable=True)
+    prebuild_cmds = variable(typ.List[str], value=[])
 
     #: .. versionadded:: 3.0
     #:
@@ -399,7 +398,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`List[str]`
     #: :default: ``[]``
-    postbuild_cmds = variable(typ.List[str], value=[], loggable=True)
+    postbuild_cmds = variable(typ.List[str], value=[])
 
     #: The name of the executable to be launched during the run phase.
     #:
@@ -414,13 +413,13 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #: .. versionchanged:: 3.7.3
     #:    Default value changed from ``os.path.join('.', self.unique_name)`` to
     #:    :class:`required`.
-    executable = variable(str, loggable=True)
+    executable = variable(str)
 
     #: List of options to be passed to the :attr:`executable`.
     #:
     #: :type: :class:`List[str]`
     #: :default: ``[]``
-    executable_opts = variable(typ.List[str], value=[], loggable=True)
+    executable_opts = variable(typ.List[str], value=[])
 
     #: .. versionadded:: 2.20
     #:
@@ -460,7 +459,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:    This field is now set automatically from the current partition's
     #:    configuration.
     container_platform = variable(field=ContainerPlatformField,
-                                  value=_NoRuntime())
+                                  value=_NoRuntime(), loggable=False)
 
     #: .. versionadded:: 3.0
     #:
@@ -472,7 +471,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`List[str]`
     #: :default: ``[]``
-    prerun_cmds = variable(typ.List[str], value=[], loggable=True)
+    prerun_cmds = variable(typ.List[str], value=[])
 
     #: .. versionadded:: 3.0
     #:
@@ -483,7 +482,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`List[str]`
     #: :default: ``[]``
-    postrun_cmds = variable(typ.List[str], value=[], loggable=True)
+    postrun_cmds = variable(typ.List[str], value=[])
 
     #: List of files to be kept after the test finishes.
     #:
@@ -503,7 +502,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #: .. versionchanged:: 3.3
     #:    This field accepts now also file glob patterns.
     #:
-    keep_files = variable(typ.List[str], value=[], loggable=True)
+    keep_files = variable(typ.List[str], value=[])
 
     #: List of files or directories (relative to the :attr:`sourcesdir`) that
     #: will be symlinked in the stage directory and not copied.
@@ -513,7 +512,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`List[str]`
     #: :default: ``[]``
-    readonly_files = variable(typ.List[str], value=[], loggable=True)
+    readonly_files = variable(typ.List[str], value=[])
 
     #: Set of tags associated with this test.
     #:
@@ -521,7 +520,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`Set[str]`
     #: :default: an empty set
-    tags = variable(typ.Set[str], value=set(), loggable=True)
+    tags = variable(typ.Set[str], value=set())
 
     #: List of people responsible for this test.
     #:
@@ -529,7 +528,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`List[str]`
     #: :default: ``[]``
-    maintainers = variable(typ.List[str], value=[], loggable=True)
+    maintainers = variable(typ.List[str], value=[])
 
     #: Mark this test as a strict performance test.
     #:
@@ -539,7 +538,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: boolean
     #: :default: :class:`True`
-    strict_check = variable(typ.Bool, value=True, loggable=True)
+    strict_check = variable(typ.Bool, value=True)
 
     #: Number of tasks required by this test.
     #:
@@ -582,7 +581,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:        number of required tasks by the test.
     #:     .. versionchanged:: 4.1
     #:        Allow :attr:`num_tasks` to be :obj:`None`.
-    num_tasks = variable(int, type(None), value=1, loggable=True)
+    num_tasks = variable(int, type(None), value=1)
 
     #: Number of tasks per node required by this test.
     #:
@@ -590,7 +589,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: integral or :class:`None`
     #: :default: :class:`None`
-    num_tasks_per_node = variable(int, type(None), value=None, loggable=True)
+    num_tasks_per_node = variable(int, type(None), value=None)
 
     #: Number of GPUs per node required by this test.
     #: This attribute is translated internally to the ``_rfm_gpu`` resource.
@@ -602,7 +601,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: .. versionchanged:: 4.0.0
     #:    The default value changed to :const:`None`.
-    num_gpus_per_node = variable(int, type(None), value=None, loggable=True)
+    num_gpus_per_node = variable(int, type(None), value=None)
 
     #: Number of CPUs per task required by this test.
     #:
@@ -610,7 +609,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: integral or :class:`None`
     #: :default: :class:`None`
-    num_cpus_per_task = variable(int, type(None), value=None, loggable=True)
+    num_cpus_per_task = variable(int, type(None), value=None)
 
     #: Number of tasks per core required by this test.
     #:
@@ -618,7 +617,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: integral or :class:`None`
     #: :default: :class:`None`
-    num_tasks_per_core = variable(int, type(None), value=None, loggable=True)
+    num_tasks_per_core = variable(int, type(None), value=None)
 
     #: Number of tasks per socket required by this test.
     #:
@@ -626,7 +625,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: integral or :class:`None`
     #: :default: :class:`None`
-    num_tasks_per_socket = variable(int, type(None), value=None, loggable=True)
+    num_tasks_per_socket = variable(int, type(None), value=None)
 
     #: Specify whether this tests needs simultaneous multithreading enabled.
     #:
@@ -634,8 +633,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: boolean or :class:`None`
     #: :default: :class:`None`
-    use_multithreading = variable(
-        typ.Bool, type(None), value=None, loggable=True)
+    use_multithreading = variable(typ.Bool, type(None), value=None)
 
     #: .. versionadded:: 3.0
     #:
@@ -646,19 +644,19 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #: :type: :class:`str` or :class:`datetime.timedelta`
     #: :default: :class:`None`
     max_pending_time = variable(type(None), typ.Duration, value=None,
-                                loggable=True, allow_implicit=True)
+                                allow_implicit=True)
 
     #: Specify whether this test needs exclusive access to nodes.
     #:
     #: :type: boolean
     #: :default: :class:`False`
-    exclusive_access = variable(typ.Bool, value=False, loggable=True)
+    exclusive_access = variable(typ.Bool, value=False)
 
     #: Always execute this test locally.
     #:
     #: :type: boolean
     #: :default: :class:`False`
-    local = variable(typ.Bool, value=False, loggable=True)
+    local = variable(typ.Bool, value=False)
 
     #: The set of reference values for this test.
     #:
@@ -721,7 +719,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
         typ.Tuple[~Deferrable, ~Deferrable, ~Deferrable],
         typ.Dict[str, typ.Dict[str, typ.Tuple[~Deferrable, ~Deferrable,
                                               ~Deferrable, ~Deferrable]]],
-        field=fields.ScopedDictField, value={}
+        field=fields.ScopedDictField, value={}, loggable=False
     )
 
     #: Require that a reference is defined for each system that this test is
@@ -734,7 +732,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #: :default: :const:`False`
     #:
     #: .. versionadded:: 4.0.0
-    require_reference = variable(typ.Bool, value=False)
+    require_reference = variable(typ.Bool, value=False, loggable=False)
 
     #:
     #: Refer to the :doc:`ReFrame Tutorials </tutorials>` for concrete usage
@@ -761,7 +759,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #:    .. versionchanged:: 3.6
     #:       The default value has changed from ``None`` to ``required``.
-    sanity_patterns = variable(_DeferredExpression)
+    sanity_patterns = variable(_DeferredExpression, loggable=False)
 
     #: Patterns for verifying the performance of this test.
     #:
@@ -780,7 +778,8 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:    <reframe.core.builtins.performance_function>` builtin or the
     #:    :attr:`perf_variables`, as :attr:`perf_patterns` will likely be
     #:    deprecated in the future.
-    perf_patterns = variable(typ.Dict[str, _DeferredExpression], type(None))
+    perf_patterns = variable(typ.Dict[str, _DeferredExpression], type(None),
+                             loggable=False)
 
     #: The performance variables associated with the test.
     #:
@@ -816,7 +815,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: .. versionadded:: 3.8.0
     perf_variables = variable(typ.Dict[str, _DeferredPerformanceExpression],
-                              value={})
+                              value={}, loggable=False)
 
     #: List of modules to be loaded before running this test.
     #:
@@ -825,7 +824,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #: :type: :class:`List[str]` or :class:`Dict[str, object]`
     #: :default: ``[]``
     modules = variable(typ.List[str], typ.List[typ.Dict[str, object]],
-                       value=[], loggable=True)
+                       value=[])
 
     #: Environment variables to be set before running this test.
     #:
@@ -837,7 +836,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: .. versionadded:: 4.0.0
     env_vars = variable(typ.Dict[str, str],
-                        typ.Dict[str, object], value={}, loggable=True)
+                        typ.Dict[str, object], value={})
     # NOTE: We still keep the original type, just to allow setting this
     # variable from the command line, because otherwise, ReFrame will not know
     # how to convert a value to an arbitrary object.
@@ -848,7 +847,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: .. deprecated:: 4.0.0
     #:    Please use :attr:`env_vars` instead.
-    variables = deprecate(variable(alias=env_vars, loggable=True),
+    variables = deprecate(variable(alias=env_vars),
                           f"the use of 'variables' is deprecated; "
                           f"please use 'env_vars' instead")
 
@@ -880,7 +879,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:       The default value is now :class:`None` and it can be set globally
     #:       per partition via the configuration.
     time_limit = variable(type(None), typ.Duration, value=None,
-                          loggable=True, allow_implicit=True)
+                          allow_implicit=True)
 
     #: .. versionadded:: 3.5.1
     #:
@@ -891,7 +890,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #: :type: :class:`str` or :class:`float` or :class:`int`
     #: :default: :class:`None`
     build_time_limit = variable(type(None), typ.Duration, value=None,
-                                loggable=True, allow_implicit=True)
+                                allow_implicit=True)
 
     #: .. versionadded:: 2.8
     #:
@@ -964,7 +963,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:       A new more powerful syntax was introduced
     #:       that allows also custom job script directive prefixes.
     extra_resources = variable(typ.Dict[str, typ.Dict[str, object]],
-                               value={}, loggable=True)
+                               value={})
 
     #: .. versionadded:: 3.3
     #:
@@ -980,7 +979,7 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: boolean
     #: :default: :class:`True`
-    build_locally = variable(typ.Bool, value=True, loggable=True)
+    build_locally = variable(typ.Bool, value=True)
 
     #: .. versionadded:: 4.2
     #:
@@ -1003,12 +1002,13 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable):
     #:
     #: :type: :class:`dict`
     #: :default: ``{}``
-    ci_extras = variable(typ.Dict[typ.Str['gitlab'], object], value={})
+    ci_extras = variable(typ.Dict[typ.Str['gitlab'], object], value={},
+                         loggable=False)
 
     # Special variables
 
     #: Dry-run mode
-    _rfm_dry_run = variable(typ.Bool, value=False)
+    _rfm_dry_run = variable(typ.Bool, value=False, loggable=False)
 
     def __new__(cls, *args, **kwargs):
         obj = super().__new__(cls)
diff --git a/reframe/core/variables.py b/reframe/core/variables.py
index 09392061..91c54aa8 100644
--- a/reframe/core/variables.py
+++ b/reframe/core/variables.py
@@ -178,7 +178,8 @@ class TestVar:
         field can be specified for an alias variable.
     :param loggable: Mark this variable as loggable. If :obj:`True`, this
         variable will become a log record attribute under the name
-        ``check_NAME``, where ``NAME`` is the name of the variable.
+        ``check_NAME``, where ``NAME`` is the name of the variable (default
+        :obj:`True`).
     :param `kwargs`: keyword arguments to be forwarded to the constructor of
         the field validator.
     :returns: A new test variable.
@@ -189,6 +190,9 @@ class TestVar:
     .. versionadded:: 4.0.0
        Alias variable are introduced.
 
+    .. versionchanged:: 4.5
+       Variables are now loggable by default.
+
     '''
 
     # NOTE: We can't use truly private fields in `__slots__`, because
@@ -220,7 +224,7 @@ class TestVar:
         else:
             self._p_default_value = kwargs.pop('value', Undefined)
 
-        self._loggable = kwargs.pop('loggable', False)
+        self._loggable = kwargs.pop('loggable', True)
         if not issubclass(field_type, fields.Field):
             raise TypeError(
                 f'field {field_type!r} is not derived from '


Test patch:
diff --git a/unittests/resources/checks/frontend_checks.py b/unittests/resources/checks/frontend_checks.py
index 7923672c..d0e7205b 100644
--- a/unittests/resources/checks/frontend_checks.py
+++ b/unittests/resources/checks/frontend_checks.py
@@ -25,7 +25,7 @@ class BaseFrontendCheck(rfm.RunOnlyRegressionTest):
 
     # Add two required variables that may affect the final run report
     x = variable(int)
-    xlog = variable(int, loggable=True)
+    xlog = variable(int)
 
     @run_after('setup')
     def setx(self):
diff --git a/unittests/test_logging.py b/unittests/test_logging.py
index 7fce8e58..97009298 100644
--- a/unittests/test_logging.py
+++ b/unittests/test_logging.py
@@ -23,18 +23,16 @@ from reframe.core.schedulers import Job
 @pytest.fixture
 def fake_check():
     class _FakeCheck(rfm.RegressionTest):
-        param = parameter(range(3), loggable=True, fmt=lambda x: 10*x)
-        custom = variable(str, value='hello extras', loggable=True)
-        custom2 = variable(alias=custom)
-        custom_list = variable(list,
-                               value=['custom', 3.0, ['hello', 'world']],
-                               loggable=True)
-        custom_dict = variable(dict, value={'a': 1, 'b': 2}, loggable=True)
+        param = parameter(range(3), fmt=lambda x: 10*x)
+        custom = variable(str, value='hello extras')
+        custom2 = variable(alias=custom, loggable=False)
+        custom_list = variable(list, value=['custom', 3.0, ['hello', 'world']])
+        custom_dict = variable(dict, value={'a': 1, 'b': 2})
 
         # x is a variable that is loggable, but is left undefined. We want to
         # make sure that logging does not crash and simply reports is as
         # undefined
-        x = variable(str, loggable=True)
+        x = variable(str)
 
     # A bit hacky, but we don't want to run a full test every time
     test = _FakeCheck(variant_num=1)
diff --git a/unittests/test_meta.py b/unittests/test_meta.py
index 060df0bf..7f868f21 100644
--- a/unittests/test_meta.py
+++ b/unittests/test_meta.py
@@ -512,10 +512,10 @@ def test_get_variant_nums(MyMeta):
 
 def test_loggable_attrs():
     class T(metaclass=meta.RegressionTestMeta):
-        x = variable(int, value=3, loggable=True)
-        y = variable(int, loggable=True)    # loggable but undefined
-        z = variable(int)
-        p = parameter(range(3), loggable=True)
+        x = variable(int, value=3)
+        y = variable(int)    # loggable but undefined
+        z = variable(int, loggable=False)
+        p = parameter(range(3))
 
         @loggable
         @property
@@ -555,6 +555,6 @@ def test_inherited_loggable_attrs():
 
 def test_deprecated_loggable_attrs():
     class T(metaclass=meta.RegressionTestMeta):
-        x = deprecate(variable(int, value=3, loggable=True), 'deprecated')
+        x = deprecate(variable(int, value=3), 'deprecated')
 
     assert T.loggable_attrs() == [('x', None)]