Skip to content

Conversation

@ChristopherBignamini
Copy link
Contributor

@ChristopherBignamini ChristopherBignamini commented Jan 30, 2019

@vkarak I'm still using the search method instead of the match one because I have the impression that in this way we can provide more flexibility to the user, who could get a "match" behavior by adding the ^ special character to his regexp.

Fixes #69.

@pep8speaks
Copy link

pep8speaks commented Jan 30, 2019

Hello @ChristopherBignamini, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2019-03-15 08:59:08 UTC

@ChristopherBignamini ChristopherBignamini changed the title Check filtering with regular expressions [feat] Check filtering with regular expressions Jan 30, 2019
@ChristopherBignamini
Copy link
Contributor Author

I forgot to mention that for the moment I have left all the old selection options, I will remove them when the new regexp based ones will have a final form (the old ones are useful for debugging)

@vkarak vkarak requested review from teojgo and victorusu January 30, 2019 13:31
@vkarak vkarak changed the title [feat] Check filtering with regular expressions [wip] [feat] Check filtering with regular expressions Jan 30, 2019
@codecov-io
Copy link

codecov-io commented Jan 30, 2019

Codecov Report

Merging #664 into master will decrease coverage by <.01%.
The diff coverage is 96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #664      +/-   ##
==========================================
- Coverage   91.83%   91.82%   -0.01%     
==========================================
  Files          77       77              
  Lines        9494     9498       +4     
==========================================
+ Hits         8719     8722       +3     
- Misses        775      776       +1
Impacted Files Coverage Δ
unittests/test_pipeline.py 97.32% <ø> (-0.04%) ⬇️
reframe/core/pipeline.py 93.67% <100%> (+0.01%) ⬆️
reframe/frontend/check_filters.py 100% <100%> (ø) ⬆️
unittests/test_check_filters.py 100% <100%> (ø) ⬆️
reframe/frontend/cli.py 80.66% <80%> (-0.36%) ⬇️
reframe/core/exceptions.py 82.03% <0%> (-1.92%) ⬇️
reframe/utility/__init__.py 93.25% <0%> (ø) ⬆️
reframe/core/config.py 84.48% <0%> (+1.72%) ⬆️

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 d2b34e3...2564ab0. Read the comment docs.

@vkarak vkarak added this to the Upcoming sprint milestone Feb 7, 2019
vkarak
vkarak previously requested changes Feb 7, 2019
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.

Can you change the existing filter functions. It would be easier to see the exact differences. Thanks to git we can retrieve the current implementation easily.

@vkarak
Copy link
Contributor

vkarak commented Feb 14, 2019

@ChristopherBignamini Unit tests are broken. Can you fix them before the review?

@vkarak
Copy link
Contributor

vkarak commented Feb 15, 2019

@jenkins-cscs retry daint

@ChristopherBignamini ChristopherBignamini changed the title [wip] [feat] Check filtering with regular expressions [feat] Check filtering with regular expressions Feb 19, 2019
@vkarak vkarak dismissed their stale review February 20, 2019 00:06

There are more fundamental issues still

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.

There are still a couple of issues in the API, so I'm not going to do a detailed review of the implementation yet. I will come back again with more details.

@sekelle sekelle removed this from the ReFrame sprint 2019w07 milestone Feb 25, 2019
@vkarak vkarak changed the title [feat] Check filtering with regular expressions [feat] Add support for selecting tests using regular expressions Feb 27, 2019
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.

I have worked a bit on the API and I have some general comments/suggestions:

  1. The filter functions must take a regex pattern string, not a regex object.
  2. The filter functions will compile the regex object in the closure of the actual filter functions that will be passed subsequently to the filter() function.
  3. The filter functions should remain simple. They will only take a single regex pattern as argument. The CLI will do the multiple filterings for each of the options passed.
  4. Multiple -n and -x options specified in the command line are essentially OR-ed (or AND-ed for the -x option). With the support for regular expressions, supporting passing -n and -x multiple times is redundant, since -n A -n B is equivalent of -n 'A|B' and -x A -x B is equivalent of -x 'A|B'. Since we cannot drop the support for multiple -n and -x options for the moment, we can aggregate them in a single regex before passing them to the filtering functions. We should decide whether we will drop support for multiple -n and -x options in the future and, therefore, we need also to issue a deprecation warning.
  5. Filtering per programming environments. There is a major complication with this right now and this comes from the fact that we support wildcards for the valid_prog_environs attribute. Ideally, we would need a way to figure out that the set of values covered by the valid_prog_environs patterns is a subset of the values covered by the regex passed in the -p option. This is not trivial at all! So I suggest to completely drop the support of wildcards for the valid_prog_environs except for the * special value that denotes any environment. I was never warm in supporting wildcards in valid_prog_environs and now there is a good reason to drop them. We are not using them in any case. As soon as we drop them, we can write the filtering function in the same way that we would do that for the tags, for example.

Here is a proposal of the implementation of the filtering functions (not tested):

def have_name(patt):
    regex = re.compile(patt)
    def _fn(c):
        return c if regex.match(c.name) else None

    return _fn


def have_not_name(names):
    def _fn(c):
        return not have_name(names)(c)

    return _fn


def have_prgenv(patt):
    regex = re.compile(patt)
    def _fn(c):
        if '*' in c.valid_prog_environs:
            return c
        else:
            return c if any(regex.match(p) for p in c.valid_prog_environs)

    return _fn

    
def have_tag(patt):
    regex = re.compile(patt)
    def _fn(c):
        return c if any(regex.match(p) for p in c.tags)

    return _fn

# inside the CLI
def main():
    ...
    for t in options.tags:
        checks_matched = filter(have_tag(t), checks_matched)

    # similarly for the other options, except the `-n` and `-x`.

@victorusu @teojgo Please give some feedback on my comments above.

@vkarak vkarak added this to the Upcoming Sprint milestone Feb 28, 2019
@vkarak
Copy link
Contributor

vkarak commented Mar 1, 2019

As @ChristopherBignamini pointed out, my suggestion has a small mistake. The internal _fn() functions should return a boolean.

@ChristopherBignamini
Copy link
Contributor Author

@jenkins-cscs retry daint

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

lgtm

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.

Looks good overall and works nicely! My comments are minor. What we still miss is to remove support for wildcards in the definition of the valid_prog_environs. It should only accept the special value *. Additional to this is to update the documentation of the valid_prog_environs attribute.

@vkarak
Copy link
Contributor

vkarak commented Mar 12, 2019

@ChristopherBignamini Please try to fix the coding style issues reported by @pep8speaks.

@vkarak
Copy link
Contributor

vkarak commented Mar 14, 2019

@teojgo Can you also approve?

Co-Authored-By: ChristopherBignamini <bignamini@cscs.ch>
@vkarak vkarak merged commit 64b8f37 into reframe-hpc:master Mar 15, 2019
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.

7 participants