Support for version compatibility check #287

rsarm · 2018-05-23T09:20:49Z

Closes #166

vkarak

The details of the implementation look ok. Some more robust error checking is needed. I also propose an alternative design using a factory for nicely integrating the two different types of validation.

Since now the new style checks are merged in, we should expand this PR to totally implement the feature.

vkarak · 2018-05-28T11:31:19Z

reframe/utility/versioning.py

+        self._operations = {
+            ">": lambda x, y: x > y,
+            ">=": lambda x, y: x >= y,
+            "<": lambda x, y: x < y,


Please align all lambdas here.

vkarak · 2018-05-28T11:31:51Z

reframe/utility/versioning.py

+    def validate(self, version_ref):
+        try:
+            res = self._validate_interval(version_ref)
+        except ValueError:


This hides possible programming errors.

The try/except here or an if/else is a bit complicated here. We could go for an alternative, more OOP, design using a factory:

class _ValidatorImpl: @abc.abstractmethod def validate(version): pass class _IntervalValidator(_ValidatorImpl): def __init__(self, condition): # parse and validate the interval condition here def validate(version): # validate version against interval class _RelationalValidator(_ValidatorImpl): def __init__(self, condition): # parse and validate the relational condition here def validate(version): # validate version against the condition class VersionValidator: def __new__(cls, condition): if '..' in condition: return _IntervalValidator(condition) else: return _RelationalValidator(condition)

vkarak · 2018-05-28T11:33:02Z

reframe/utility/versioning.py

+        return Version(str_version), operator
+
+    def _validate_interval(self, version_ref):
+        min_version_str, max_version_str = self.condition.split(',')


I prefer using .. for specifying intervals. I was confused when I've seen it in the unit tests.

This statement might also through a ValueError for strings such as 1,2,3,4. You should write it as follows:

min_version_str, max_version_str, *_ = self.condition.split(',')

vkarak · 2018-05-28T11:33:27Z

reframe/utility/versioning.py

        return base + '-dev%s' % self._dev_number
+
+
+class VersionValidator:


Please add docstring describing the purpose of this class.

Similarly, for the Version class.

Using my proposed design, the docstring for this class should separated in the two implementations.

vkarak · 2018-05-28T11:42:46Z

reframe/utility/versioning.py

+        except ValueError:
+            raise ValueError('invalid interval: "%s", '
+                             'expecting "min_version, max_version"'
+                             % self.condition.strip()) from None


This reraise here is not correct, because if ValueError is thrown, it means that the version strings are wrong, sot the message is deceiving. You should not catch it at all.

vkarak · 2018-05-28T11:44:43Z

reframe/utility/versioning.py

+                             % self.condition.strip()) from None
+
+        return ((Version(version_ref) > min_version) and
+                (Version(version_ref) < max_version))


It is more intuitive for the intervals to be inclusive. When I saw the corresponding unit tests, I was expecting it to validate.

vkarak · 2018-05-28T11:45:29Z

unittests/test_versioning.py

+        self.assertTrue(any(c.validate('2.2') for c in conditions))
+        self.assertFalse(any(c.validate('2.5') for c in conditions))
+        self.assertTrue(any(c.validate('3.0') for c in conditions))
+        self.assertFalse(any(c.validate('3.1') for c in conditions))


There are no tests about things going wrong (invalid versions, invalid operators, invalid intervals).

vkarak · 2018-05-28T12:02:46Z

reframe/utility/versioning.py

+                (Version(version_ref) < max_version))
+
+    def _validate_relation(self, version_ref):
+        version, op = self._parse_condition(self.condition)


This is too late to parse and validate the condition. It should fail early in the constructor.

rsarm · 2018-06-07T09:03:19Z

@jenkins-cscs retry daint dom

vkarak · 2018-06-07T11:54:51Z

reframe/utility/versioning.py

+            min_version_str, max_version_str = condition.split('..')
+        except ValueError:
+            raise ValueError("invalid format of version interval: %s"
+                             % condition) from None


We split after the binary operators.

vkarak · 2018-06-07T11:55:03Z

reframe/utility/versioning.py

+            self.max_version = Version(max_version_str)
+        else:
+            raise ValueError("missing bound on version interval %s"
+                             % condition)


We split after the binary operators.

vkarak · 2018-06-07T11:56:01Z

reframe/utility/versioning.py

+
+    def validate(self, version):
+        return ((Version(version) >= self.min_version) and
+                (Version(version) <= self.max_version))


Convert the version string once:

version = Version(version)

Another question: Is there a reason that min_version and max_version are public?

vkarak · 2018-06-07T12:08:48Z

reframe/utility/versioning.py

+            raise ValueError('invalid condition: %s'
+                             % condition.strip()) from None
+
+        self.ref_version = Version(str_version)


I think you can write this in a simpler way using regexes (not like the first version of this PR, though):

cond_match = re.match(r'(\W{1,2})(\S+)', condition) if not cond_match: raise ValueError('invalid condition: %s' % condition) self.ref_version = Version(cond_match.group(2)) # First check, then assign. Otherwise, the self.operator will have an incorrect value. op = cond_match.group(1) if op not in self._op_actions.keys(): self.operator = op

Another question: Is there a reason that operator and ref_version are public?

vkarak · 2018-06-07T12:10:48Z

reframe/utility/versioning.py

+    ``True`` if a given version string satisfies the relation.
+    """
+    def __init__(self, condition):
+        self._operations = {


Better name this as self._op_actions

rsarm · 2018-06-08T07:20:07Z

@jenkins-cscs retry all

rsarm · 2018-06-08T13:58:45Z

@vkarak I wasn't very sure about which message to print when skipping a test and where to put it.

codecov-io · 2018-06-08T14:34:25Z

Codecov Report

Merging #287 into master will increase coverage by 0.08%.
The diff coverage is 98.07%.

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
+ Coverage   91.18%   91.27%   +0.08%     
==========================================
  Files          67       68       +1     
  Lines        7941     8042     +101     
==========================================
+ Hits         7241     7340      +99     
- Misses        700      702       +2

Impacted Files	Coverage Δ
...sts/resources/checks_unlisted/required_versions.py	`100% <100%> (ø)`
reframe/core/decorators.py	`98.21% <100%> (+0.91%)`	⬆️
unittests/test_versioning.py	`100% <100%> (ø)`	⬆️
reframe/utility/versioning.py	`89.02% <95.12%> (+5.69%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd18a5e...46e86af. Read the comment docs.

vkarak · 2018-06-11T16:08:30Z

reframe/core/decorators.py

 import collections
 import inspect

+from reframe import VERSION


Use import reframe here and reference version as reframe.VERSION.

vkarak · 2018-06-11T16:12:37Z

reframe/core/decorators.py

+from reframe import VERSION
 from reframe.core.exceptions import ReframeSyntaxError
 from reframe.core.pipeline import RegressionTest
+from reframe.frontend.printer import PrettyPrinter


A couple of things here:

You break a key design concept of the whole framework. The core should never use the frontend.

The PrettyPrinter is meant to be used by the frontend and the execution policies for nicely formatting the output. Internally, it uses a logger to do the actual "printing".

The core uses the low-level logger to log stuff.

vkarak · 2018-06-11T16:18:06Z

reframe/core/decorators.py

+        if not any(c.validate(VERSION) for c in conditions):
+            printer.status('SKIP',
+                           'skipping uncompatible class %s' % cls.__name__,
+                           just='center')


You should not be using the pretty printer here. Use instead:

from reframe.core.logging import getlogger getlogger().info('skipping incompatible test defined in class: %s' % cls.__name__)

vkarak · 2018-06-11T16:19:06Z

reframe/core/decorators.py

+                           'skipping uncompatible class %s' % cls.__name__,
+                           just='center')
+            try:
+                mod.__rfm_skip_tests |= {cls}


I'd use add() here.

vkarak · 2018-06-11T16:21:13Z

reframe/core/decorators.py

+
+
+def required_version(*compat_versions):
+    printer = PrettyPrinter()


Statement before the docstring??

vkarak · 2018-06-11T21:29:44Z

reframe/core/decorators.py

+
+def required_version(*compat_versions):
+    printer = PrettyPrinter()
+    """Class decorator for skipping version-uncompatible tests."""


This should be documented properly. It's a public function.

vkarak · 2018-06-11T21:38:09Z

reframe/core/decorators.py

+            try:
+                mod.__rfm_skip_tests |= {cls}
+            except AttributeError:
+                mod.__rfm_skip_tests = {cls}


I'd move this try/except outside this if:

mod = inspect.getmodule(cls) if not hasattr(mod, '__rfm_skip_tests'): mod.__rfm_skip_tests = set() if not any(c.validate(reframe.VERSION) for c in conditions): mod.__rfm_skip_tests.add(cls) ...

vkarak · 2018-06-11T21:41:35Z

reframe/utility/versioning.py

+
+        self._ref_version = Version(cond_match.group(2))
+        op = cond_match.group(1)
+        if op == '':


vkarak · 2018-06-11T21:42:51Z

reframe/utility/versioning.py

+
+    def validate(self, version):
+        return self._op_actions[self._operator](Version(version),
+                                                self._ref_version)


Better write this as follows:

do_validate = self._op_actions[self._operator] return do_validate(Version(version), self._ref_version)

vkarak · 2018-06-11T21:44:26Z

unittests/test_versioning.py

+        self.assertRaises(ValueError, VersionValidator, '2.0.0>')
+        self.assertRaises(ValueError, VersionValidator, '2.0.0>1.0.0')
+        self.assertRaises(ValueError, VersionValidator, '=>')
+        self.assertRaises(ValueError, VersionValidator, '>1')


We will need more unit tests for the new decorator.

vkarak · 2018-06-18T13:03:02Z

unittests/resources/checks_unlisted/required_versions.py

+    pass
+
+
+@rfm.required_version()


I don't think we should allow this.

vkarak · 2018-06-18T13:04:04Z

tutorial/example1.py

 import reframe.utility.sanity as sn

-
+@rfm.required_version()


Remove this from here.

vkarak · 2018-06-19T07:19:06Z

unittests/resources/checks_unlisted/required_versions.py

+@rfm.simple_test
+@rfm.required_version('!=2.0')
+class Test5Check(rfm.RegressionTest):
+    pass


I don't think we need this file at all. We already have the unittests/resources/checks_unlisted/good.py that is being loaded by the check loader. We could simply decorate one of the tests in there with a required_version that is always true and add another test, which should be skipped.

vkarak · 2018-06-19T07:26:00Z

reframe/core/decorators.py

+
+
+def required_version(*versions):
+    """Class decorator for skipping version-uncompatible tests.


Class decorator for specifying the required ReFrame version for the following test.

vkarak · 2018-06-19T07:27:33Z

reframe/core/decorators.py

+    :arg versions: The versions that are compatible with the test.
+
+    .. versionadded:: 2.13
+


Remove this blank line.

vkarak · 2018-06-19T07:27:58Z

reframe/core/decorators.py

+
+    The decorated class must derive from
+    :class:`reframe.core.pipeline.RegressionTest`. This decorator is also
+    available directly under the :mod:`reframe` module.


I don't think you test for that! And I don't think you should in any case...

vkarak · 2018-06-19T07:28:45Z

reframe/core/decorators.py

+    :class:`reframe.core.pipeline.RegressionTest`. This decorator is also
+    available directly under the :mod:`reframe` module.
+
+    :arg versions: The versions that are compatible with the test.


Please explain a bit the syntax here. Also you should specify the type of the argument. See other part of the documentation on how this is done.

Also: - Complete documentation of the `require_version` decorator. - More robust algorithm for identifying problematic user code.

vkarak · 2018-06-19T14:05:54Z

@jenkins-cscs retry daint

vkarak · 2018-06-19T14:42:08Z

@jenkins-cscs retry daint

First commit

e300e9b

rsarm added request for enhancement prio: normal enhancement labels May 23, 2018

rsarm added this to the ReFrame sprint 2018w20 milestone May 23, 2018

rsarm self-assigned this May 23, 2018

vkarak self-requested a review May 28, 2018 09:35

vkarak changed the title ~~Version compatibility~~ Support for version compatibility checks May 28, 2018

vkarak requested changes May 28, 2018

View reviewed changes

vkarak changed the title ~~Support for version compatibility checks~~ Support for version compatibility check May 28, 2018

Fixing comments

e021282

vkarak modified the milestones: ReFrame sprint 2018w20, Upcoming sprint Jun 4, 2018

vkarak reviewed Jun 7, 2018

View reviewed changes

rsarm added 3 commits June 7, 2018 15:29

PR fixes

fe5ceaf

Merge branch 'master' into enhancement/version-compatibility

4afa538

PR fixes

2e180b4

rsarm added 2 commits June 8, 2018 09:55

Merge branch 'master' into enhancement/version-compatibility

6c04c72

Adding required_version decorator

0bb1b52

vkarak requested changes Jun 11, 2018

View reviewed changes

PR fixes

46e86af

vkarak requested changes Jun 19, 2018

View reviewed changes

vkarak self-assigned this Jun 19, 2018

Vasileios Karakasis added 2 commits June 19, 2018 14:41

Merge branch 'master' into enhancement/version-compatibility

7b3af31

Fine tune versioning implementation

bfead90

Also: - Complete documentation of the `require_version` decorator. - More robust algorithm for identifying problematic user code.

vkarak approved these changes Jun 19, 2018

View reviewed changes

vkarak merged commit 527907f into master Jun 19, 2018

vkarak deleted the enhancement/version-compatibility branch June 19, 2018 15:12

		return base + '-dev%s' % self._dev_number


		class VersionValidator:



		def required_version(*compat_versions):
		printer = PrettyPrinter()



		def required_version(*versions):
		"""Class decorator for skipping version-uncompatible tests.

		:arg versions: The versions that are compatible with the test.

		.. versionadded:: 2.13

		pass


		@rfm.required_version()

Support for version compatibility check #287

Support for version compatibility check #287

Uh oh!

Conversation

rsarm commented May 23, 2018 • edited by vkarak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkarak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkarak May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsarm commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsarm commented Jun 8, 2018

Uh oh!

rsarm commented Jun 8, 2018

Uh oh!

codecov-io commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsarm commented May 23, 2018 •

edited by vkarak

Loading

vkarak May 28, 2018 •

edited

Loading

codecov-io commented Jun 8, 2018 •

edited

Loading