Skip to content

Conversation

@victorusu
Copy link
Contributor

@victorusu victorusu commented Feb 19, 2020

Instead of failing when comparing reference of different types with

TypeError: '>=' not supported between instances of 'str' and 'float'

now, ReFrame fails with a much nicer message like (edited by @vkarak)

the value extracted for performance variable 'generic:login:filename' is not a number: 9.c

Closes #1146

Instead of failing when comparing reference of different types with
```
TypeError: '>=' not supported between instances of 'str' and 'float'
```
now, ReFrame fails with a much nicer message like
```
cannot compare 9.c with reference value 0
```
@pep8speaks
Copy link

pep8speaks commented Feb 19, 2020

Hello @victorusu, Thank you for updating!

Line 1325:36: E127 continuation line over-indented for visual indent
Line 1325:51: E231 missing whitespace after ':'
Line 1325:80: E501 line too long (92 > 79 characters)
Line 1326:80: E501 line too long (90 > 79 characters)
Line 1328:80: E501 line too long (89 > 79 characters)

Do see the ReFrame Coding Style Guide

Comment last updated at 2020-02-24 14:36:00 UTC

@vkarak vkarak changed the title [bug] Fix TypeError message for performance report [bugfix] Fix TypeError message for performance report Feb 19, 2020
vkarak
vkarak previously requested changes Feb 20, 2020
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from my other comments, we will need also a unit test to test this.

@vkarak vkarak self-assigned this Feb 24, 2020
Vasileios Karakasis added 3 commits February 24, 2020 10:44
The reason for that is that we don't do any argument type checking in any other
sanity function, so it would be strange to do it in either `assert_reference()`
or `assert_bounded()` only. The reason for that is that type and value errors
are handled specially if they are raised from user test context. In this
particular case, though, the `assert_reference()` is called from the framework
context as a result of user input. So it makes more sense then to do the type
checking inside `check_performance()`, where we call it.

We also don't type check the reference and lower/upper thresholds, because this
should be addressed correctly by the updated declaration of the `reference`
field as soon as reframe-hpc#1147 is fixed (see
also related FIXME note for the `reference` field).

Finally, this commit adds also a pipeline unit test to check for this condition.
@vkarak
Copy link
Contributor

vkarak commented Feb 24, 2020

@victorusu I have changed the implementation. I am quoting the comment of my last commit:

    Move checking of the perf var extracted variable inside the pipeline

    The reason for that is that we don't do any argument type checking in any other
    sanity function, so it would be strange to do it in either `assert_reference()`
    or `assert_bounded()` only. The reason for that is that type and value errors
    are handled specially if they are raised from user test context. In this
    particular case, though, the `assert_reference()` is called from the framework
    context as a result of user input. So it makes more sense then to do the type
    checking inside `check_performance()`, where we call it.

    We also don't type check the reference and lower/upper thresholds, because this
    should be addressed correctly by the updated declaration of the `reference`
    field as soon as https://github.com/eth-cscs/reframe/issues/1147 is fixed (see
    also related FIXME note for the `reference` field).

    Finally, this commit adds also a pipeline unit test to check for this condition.

@vkarak
Copy link
Contributor

vkarak commented Feb 24, 2020

@teojgo Can you review this PR?

@vkarak vkarak dismissed their stale review February 24, 2020 14:49

I changed completely the implementation, so I need another reviewer

@vkarak vkarak changed the title [bugfix] Fix TypeError message for performance report [bugfix] Fix TypeError crash in the performance checking phase when the extracted performance variable value is not a number Feb 24, 2020
@vkarak vkarak merged commit 423320e into reframe-hpc:master Feb 24, 2020
@victorusu victorusu deleted the framework/perf-value-str-fail-nicely branch October 6, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

performance report does not accept str

4 participants