From 397fdce236603ac223fbb33efd62be79b9ae0e87 Mon Sep 17 00:00:00 2001 From: Theofilos Manitaras Date: Wed, 2 Dec 2020 17:00:54 +0100 Subject: [PATCH 1/3] Drop support of pipeline stage method override --- docs/migration_2_to_3.rst | 9 +++- reframe/core/meta.py | 8 ++-- reframe/core/pipeline.py | 64 +++++++++++++++++++++++++++ unittests/test_loader.py | 93 ++++++++++++++++----------------------- 4 files changed, 114 insertions(+), 60 deletions(-) diff --git a/docs/migration_2_to_3.rst b/docs/migration_2_to_3.rst index ad031afa87..8293f549ce 100644 --- a/docs/migration_2_to_3.rst +++ b/docs/migration_2_to_3.rst @@ -66,6 +66,7 @@ Pipeline methods and hooks ReFrame 2.20 introduced a new powerful mechanism for attaching arbitrary functions hooks at the different pipeline stages. This mechanism provides an easy way to configure and extend the functionality of a test, eliminating essentially the need to override pipeline stages for this purpose. ReFrame 3.0 deprecates the old practice of overriding pipeline stage methods in favor of using pipeline hooks. +From ReFrame 3.4, overriding pipeline stage methods is no longer allowed. In the old syntax, it was quite common to override the ``setup()`` method, in order to configure your test based on the current programming environment or the current system partition. The following is a typical example of that: @@ -93,7 +94,7 @@ Alternatively, this example could have been written as follows: self.build_system.cflags = ['-qopenmp'] -This syntax now issues a deprecation warning. +This syntax is no longer valid and a ``ReframeSyntaxError`` is raised. Rewriting this using pipeline hooks is quite straightforward and leads to nicer and more intuitive code: .. code:: python @@ -109,6 +110,10 @@ Rewriting this using pipeline hooks is quite straightforward and leads to nicer You could equally attach this function to run after the "setup" phase with ``@rfm.run_after('setup')``, as in the original example, but attaching it to the "compile" phase makes more sense. However, you can't attach this function *before* the "setup" phase, because the ``current_environ`` will not be available and it will be still ``None``. +.. warning:: + .. versionchanged:: 3.4 + Overriding a pipeline stage method is no longer allowed and a ``ReframeSyntaxError`` is raised. + -------------------------------- Force override a pipeline method @@ -125,7 +130,7 @@ In this case, all you have to do is mark your test class as "special", and ReFra super().setup(partition, environ, **job_opts) -If you try to override the ``setup()`` method in any of the subclasses of ``MyExtendedTest``, you will still get a deprecation warning, which a desired behavior since the subclasses should be normal tests. +If you try to override the ``setup()`` method in any of the subclasses of ``MyExtendedTest``, will again result in a ``ReframeSyntaxError``, which a desired behavior since the subclasses should be normal tests. Getting schedulers and launchers by name diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 538b2393be..0e51bf8e95 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -4,10 +4,10 @@ # SPDX-License-Identifier: BSD-3-Clause # -# Met-class for creating regression tests. +# Meta-class for creating regression tests. # -from reframe.core.warnings import user_deprecation_warning +from reframe.core.exceptions import ReframeSyntaxError class RegressionTestMeta(type): @@ -57,5 +57,5 @@ def __init__(cls, name, bases, namespace, **kwargs): msg = (f"'{cls.__qualname__}.{v.__name__}' attempts to " f"override final method " f"'{b.__qualname__}.{v.__name__}'; " - f"consider using the pipeline hooks instead") - user_deprecation_warning(msg) + f"you should use the pipeline hooks instead") + raise ReframeSyntaxError(msg) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 435aded76b..92ac46ef20 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -1092,6 +1092,12 @@ def setup(self, partition, environ, **job_opts): `__ for more details. + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + ''' self._current_partition = partition self._current_environ = environ @@ -1133,6 +1139,12 @@ def compile(self): `__ for more details. + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + ''' if not self._current_environ: raise PipelineError('no programming environment set') @@ -1230,6 +1242,12 @@ def compile_wait(self): `__ for more details. + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + ''' self._build_job.wait() @@ -1252,6 +1270,13 @@ def run(self): special test. See `here `__ for more details. + + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + ''' if not self.current_system or not self._current_partition: raise PipelineError('no system or system partition is set') @@ -1357,6 +1382,13 @@ def run_complete(self): `__ for more details. + + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + ''' if not self._job: return True @@ -1387,6 +1419,14 @@ def run_wait(self): `__ for more details. + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + + + ''' self._job.wait() @@ -1428,6 +1468,14 @@ def check_sanity(self): `__ for more details. + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + + + ''' if rt.runtime().get_option('general/0/trap_job_errors'): sanity_patterns = [ @@ -1461,6 +1509,14 @@ def check_performance(self): `__ for more details. + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + + + ''' if self.perf_patterns is None: return @@ -1579,6 +1635,14 @@ def cleanup(self, remove_files=False): `__ for more details. + .. versionchanged:: 3.4 + Overriding this method directly unless you are in + special test. See `here + `__ for + more details. + + + ''' aliased = os.path.samefile(self._stagedir, self._outputdir) if aliased: diff --git a/unittests/test_loader.py b/unittests/test_loader.py index 873e4da77d..5c77a10e2e 100644 --- a/unittests/test_loader.py +++ b/unittests/test_loader.py @@ -8,9 +8,9 @@ import reframe as rfm from reframe.core.exceptions import (ConfigError, NameConflictError, - RegressionTestLoadError) + RegressionTestLoadError, + ReframeSyntaxError) from reframe.core.systems import System -from reframe.core.warnings import ReframeDeprecationWarning from reframe.frontend.loader import RegressionCheckLoader @@ -83,72 +83,62 @@ def test_load_bad_init(loader): def test_special_test(): - with pytest.warns(ReframeDeprecationWarning): + with pytest.raises(ReframeSyntaxError): @rfm.simple_test - class TestDeprecated(rfm.RegressionTest): + class TestOverride(rfm.RegressionTest): def setup(self, partition, environ, **job_opts): super().setup(system, environ, **job_opts) - with pytest.warns(ReframeDeprecationWarning): + with pytest.raises(ReframeSyntaxError): @rfm.simple_test - class TestDeprecatedRunOnly(rfm.RunOnlyRegressionTest): + class TestOverrideRunOnly(rfm.RunOnlyRegressionTest): def setup(self, partition, environ, **job_opts): super().setup(system, environ, **job_opts) - with pytest.warns(ReframeDeprecationWarning): + with pytest.raises(ReframeSyntaxError): @rfm.simple_test - class TestDeprecatedCompileOnly(rfm.CompileOnlyRegressionTest): + class TestOverrideCompileOnly(rfm.CompileOnlyRegressionTest): def setup(self, partition, environ, **job_opts): super().setup(system, environ, **job_opts) - with pytest.warns(ReframeDeprecationWarning): - @rfm.simple_test - class TestDeprecatedCompileOnlyDerived(TestDeprecatedCompileOnly): - def setup(self, partition, environ, **job_opts): - super().setup(system, environ, **job_opts) - - with pytest.warns(None) as warnings: - @rfm.simple_test - class TestSimple(rfm.RegressionTest): - def __init__(self): - pass + @rfm.simple_test + class TestSimple(rfm.RegressionTest): + def __init__(self): + pass - @rfm.simple_test - class TestSpecial(rfm.RegressionTest, special=True): - def __init__(self): - pass + @rfm.simple_test + class TestSpecial(rfm.RegressionTest, special=True): + def __init__(self): + pass - def setup(self, partition, environ, **job_opts): - super().setup(system, environ, **job_opts) + def setup(self, partition, environ, **job_opts): + super().setup(system, environ, **job_opts) - @rfm.simple_test - class TestSpecialRunOnly(rfm.RunOnlyRegressionTest, - special=True): - def __init__(self): - pass + @rfm.simple_test + class TestSpecialRunOnly(rfm.RunOnlyRegressionTest, + special=True): + def __init__(self): + pass - def setup(self, partition, environ, **job_opts): - super().setup(system, environ, **job_opts) + def setup(self, partition, environ, **job_opts): + super().setup(system, environ, **job_opts) - def run(self): - super().run() + def run(self): + super().run() - @rfm.simple_test - class TestSpecialCompileOnly(rfm.CompileOnlyRegressionTest, - special=True): - def __init__(self): - pass - - def setup(self, partition, environ, **job_opts): - super().setup(system, environ, **job_opts) + @rfm.simple_test + class TestSpecialCompileOnly(rfm.CompileOnlyRegressionTest, + special=True): + def __init__(self): + pass - def run(self): - super().run() + def setup(self, partition, environ, **job_opts): + super().setup(system, environ, **job_opts) - assert not any(isinstance(w.message, ReframeDeprecationWarning) - for w in warnings) + def run(self): + super().run() - with pytest.warns(ReframeDeprecationWarning) as warnings: + with pytest.raises(ReframeSyntaxError): @rfm.simple_test class TestSpecialDerived(TestSpecial): def __init__(self): @@ -157,21 +147,16 @@ def __init__(self): def setup(self, partition, environ, **job_opts): super().setup(system, environ, **job_opts) - def run(self): - super().run() - - assert len(warnings) == 2 - @rfm.simple_test class TestFinal(rfm.RegressionTest): def __init__(self): pass @rfm.final - def my_new_final(seld): + def my_new_final(self): pass - with pytest.warns(ReframeDeprecationWarning): + with pytest.raises(ReframeSyntaxError): @rfm.simple_test class TestFinalDerived(TestFinal): def my_new_final(self, a, b): From 841a08ad6c0c76598439bb0f1edf4b801d1a32cf Mon Sep 17 00:00:00 2001 From: Theofilos Manitaras Date: Wed, 2 Dec 2020 17:09:40 +0100 Subject: [PATCH 2/3] Remove unused imports --- unittests/test_loader.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/unittests/test_loader.py b/unittests/test_loader.py index 5c77a10e2e..9a4a3814ff 100644 --- a/unittests/test_loader.py +++ b/unittests/test_loader.py @@ -7,10 +7,7 @@ import pytest import reframe as rfm -from reframe.core.exceptions import (ConfigError, NameConflictError, - RegressionTestLoadError, - ReframeSyntaxError) -from reframe.core.systems import System +from reframe.core.exceptions import (NameConflictError, ReframeSyntaxError) from reframe.frontend.loader import RegressionCheckLoader From 0a219749d8095c447f1ad58889bd40291852edd4 Mon Sep 17 00:00:00 2001 From: Theofilos Manitaras Date: Thu, 3 Dec 2020 15:14:19 +0100 Subject: [PATCH 3/3] Address PR comments --- docs/migration_2_to_3.rst | 9 ++++----- reframe/core/pipeline.py | 37 ++++++++++--------------------------- unittests/test_loader.py | 2 +- 3 files changed, 15 insertions(+), 33 deletions(-) diff --git a/docs/migration_2_to_3.rst b/docs/migration_2_to_3.rst index 8293f549ce..426e96e12e 100644 --- a/docs/migration_2_to_3.rst +++ b/docs/migration_2_to_3.rst @@ -65,8 +65,7 @@ Pipeline methods and hooks ReFrame 2.20 introduced a new powerful mechanism for attaching arbitrary functions hooks at the different pipeline stages. This mechanism provides an easy way to configure and extend the functionality of a test, eliminating essentially the need to override pipeline stages for this purpose. -ReFrame 3.0 deprecates the old practice of overriding pipeline stage methods in favor of using pipeline hooks. -From ReFrame 3.4, overriding pipeline stage methods is no longer allowed. +ReFrame 3.0 deprecates the old practice of overriding pipeline stage methods in favor of using pipeline hooks and ReFrame 3.4 disables that by default. In the old syntax, it was quite common to override the ``setup()`` method, in order to configure your test based on the current programming environment or the current system partition. The following is a typical example of that: @@ -94,7 +93,7 @@ Alternatively, this example could have been written as follows: self.build_system.cflags = ['-qopenmp'] -This syntax is no longer valid and a ``ReframeSyntaxError`` is raised. +This syntax is no longer valid and it will raise a deprecation warning for ReFrame versions >= 3.0 and a reframe syntax error for versions >=3.4. Rewriting this using pipeline hooks is quite straightforward and leads to nicer and more intuitive code: .. code:: python @@ -112,7 +111,7 @@ However, you can't attach this function *before* the "setup" phase, because the .. warning:: .. versionchanged:: 3.4 - Overriding a pipeline stage method is no longer allowed and a ``ReframeSyntaxError`` is raised. + Overriding a pipeline stage method is no longer allowed and a reframe syntax error is raised. -------------------------------- @@ -130,7 +129,7 @@ In this case, all you have to do is mark your test class as "special", and ReFra super().setup(partition, environ, **job_opts) -If you try to override the ``setup()`` method in any of the subclasses of ``MyExtendedTest``, will again result in a ``ReframeSyntaxError``, which a desired behavior since the subclasses should be normal tests. +If you try to override the ``setup()`` method in any of the subclasses of ``MyExtendedTest``, it will again result in a reframe syntax error, which is a desired behavior since the subclasses should be normal tests. Getting schedulers and launchers by name diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 92ac46ef20..1cfcff216a 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -30,7 +30,7 @@ import reframe.utility.sanity as sn import reframe.utility.typecheck as typ import reframe.utility.udeps as udeps -from reframe.core.backends import (getlauncher, getscheduler) +from reframe.core.backends import getlauncher, getscheduler from reframe.core.buildsystems import BuildSystemField from reframe.core.containers import ContainerPlatformField from reframe.core.deferrable import _DeferredExpression @@ -1093,8 +1093,7 @@ def setup(self, partition, environ, **job_opts): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. @@ -1140,8 +1139,7 @@ def compile(self): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. @@ -1243,8 +1241,7 @@ def compile_wait(self): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. @@ -1272,8 +1269,7 @@ def run(self): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. @@ -1384,8 +1380,7 @@ def run_complete(self): .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. @@ -1420,13 +1415,10 @@ def run_wait(self): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. - - ''' self._job.wait() @@ -1469,13 +1461,10 @@ def check_sanity(self): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. - - ''' if rt.runtime().get_option('general/0/trap_job_errors'): sanity_patterns = [ @@ -1510,13 +1499,10 @@ def check_performance(self): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. - - ''' if self.perf_patterns is None: return @@ -1636,13 +1622,10 @@ def cleanup(self, remove_files=False): more details. .. versionchanged:: 3.4 - Overriding this method directly unless you are in - special test. See `here + Overriding this method directly in no longer allowed. See `here `__ for more details. - - ''' aliased = os.path.samefile(self._stagedir, self._outputdir) if aliased: diff --git a/unittests/test_loader.py b/unittests/test_loader.py index 9a4a3814ff..f5fe917eab 100644 --- a/unittests/test_loader.py +++ b/unittests/test_loader.py @@ -7,7 +7,7 @@ import pytest import reframe as rfm -from reframe.core.exceptions import (NameConflictError, ReframeSyntaxError) +from reframe.core.exceptions import NameConflictError, ReframeSyntaxError from reframe.frontend.loader import RegressionCheckLoader