[feat] Add new ci_extras test variable that allows passing test-specific options to the pipeline generated by --ci-generate

[donion74]

This PR allows to use Gitlab CI options to filter tests, e.g. based on downstream pipelines or changes of the file. A minimal working example is provided in the repository https://gitlab.com/latticeqcd/reframe-debug, see hello1.py

• Pipeline 810057836 runs for a branch that is in merge request, but has unmodified source file. Hence the test HelloMergeRequest is run.
• Pipeline 810072116 now has modified source file and thus runs additionally HelloModified.

[jenkins-cscs]

Can I test this patch?

[donion74]

@jenkins-cscs Yes, I just updated the description.

[codecov-commenter]

Codecov Report

Patch coverage: 92.85% and project coverage change: +0.01 🎉

Comparison is base (6e22add) 86.84% compared to head (918d06c) 86.85%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2833      +/-   ##
===========================================
+ Coverage    86.84%   86.85%   +0.01%     
===========================================
  Files           60       60              
  Lines        11402    11414      +12     
===========================================
+ Hits          9902     9914      +12     
  Misses        1500     1500              

Impacted Files	Coverage Δ
reframe/frontend/ci.py	95.55% <92.30%> (+1.43%)	⬆️
reframe/core/pipeline.py	93.89% <100.00%> (+<0.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[vkarak]

Hi @donion74, thanks for the PR!

I like the idea as it's simple, yet powerful. I think it essentially addresses what #2420 tries to do. @mahendrapaipuri Wdyt? Would that feature work for your case as well?

Other than that we'd need a unit test or expand the existing one in test_ci.py that basically exercises also this feature and a bit of fine tuning of the docs as this is not about adding "options" to the CI: users can essentially extend per test basis the generated child pipeline.

[mahendrapaipuri]

@donion74 @vkarak I think it should work in my case as well. From what I see we can arbitrary configure the child pipeline within each "test", which was exactly what we were looking for. I really like this PR as it is a very simple and efficient solution.

@donion74 What happens if we want to configure directives that are explicitly generated by current CI code? For instance, ReFrame already emits artifacts to include the report file. If user wants to add more artifacts to it, with this PR, the user can set ci_options as

ci_options = {"artifacts": {"paths": ['/path1/to/save', '/path2/to/save']}}

which will end up generating a YAML file as follows in your example:

HelloModified:
  stage: rfm-stage-0
  script:
  - reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA}  -c /builds/latticeqcd/reframe-debug  --report-file=HelloModified-report.json  --report-junit=HelloModified-report.xml  -n '^HelloModified$' -r
  artifacts:
    paths:
    - HelloModified-report.json
  needs: []
  artifacts:
   paths:
     - /path1/to/save
     - /path2/to/save

So, the question is if the GitLab CI parser (in future if other CI engines will be supported, their parsers as well) is smart enough to merge them? Could you maybe do a quick test?

[donion74]

@mahendrapaipuri Good point. If I try to reproduce your suggestions, the artifacts key is overwritten:

    ci_extras['gitlab'] = {**ci_extras['gitlab'], 
        "rules": [{"changes": ["src/hello.c"]}],
        'artifacts': {"paths": ['pipeline.yml', '.gitlab-ci.yml']}}

produces this pipeline:

HelloModified:
  stage: rfm-stage-0
  script:
  - reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C /home/anian/phd/2-openqxd/tests/config//settings.py -c . -R --report-file=HelloModified-report.json  --report-junit=HelloModified-report.xml  -n '^HelloModified$' -r
  artifacts:
    paths:
    - pipeline.yml
    - .gitlab-ci.yml
  needs: []
  rules:
  - changes:
    - src/hello.c

I guess this is related to https://stackoverflow.com/q/14902299, and could be solved by the suggestion https://stackoverflow.com/a/61416215.

[donion74]

I updated the example git repo. Here are two pipelines that demonstrate the use of changes:

• https://gitlab.com/latticeqcd/reframe-debug/-/pipelines/821682420
• https://gitlab.com/latticeqcd/reframe-debug/-/pipelines/821685043

[mahendrapaipuri]

Thanks @donion74 for the tests. So, if Gitlab CI is not merging them, we need to do it within ReFrame. So, currently ReFrame is emitting pipelines using directives stage, script, artifacts and needs.

• stage and needs are important for dependency resolution. So, if user defined ci_extras have these directives we need to ignore them
• Document clearly that users need to use before_script and after_script for executing auxillary commands within a job. If user still passes script directive within ci_extras, we need to ignore it as we do not know whether to execute them before or after ReFrame job. A warning would be nice to have when this case arises.
• artifacts need to be merged.

I think this should do nicely to configure CI pipelines. What do you think @vkarak?

[vkarak]

@mahendrapaipuri I agree with your suggestion. For needs, stage and script we need to issue an error I think and if the user passes script specifically, we should ask her to set either the before_script or after_script instead. artifacts should indeed be merged, but we should not bother implementing a generic JSON object merger but rather a specific one for the artifacts; reframe knows exactly what it emits for artifacts, so it can combine it with the user's artifacts accordingly.

[vkarak]

I updated the PR implementing the restrictions we just discussed and added also unit tests. Now lgtm. @donion74 @mahendrapaipuri If you confirm that it works for your use cases, then we can merge this PR.

[mahendrapaipuri]

@vkarak Cheers @vkarak!! Yes, this would work for my cases and this PR is quite handy. Cheers @donion74 !!

[donion74]

Thanks for implementing the changes @vkarak , my pipeline still works as intended: https://gitlab.com/latticeqcd/reframe-debug/-/pipelines/827286511

The only downside I notice is that subsequent tests overwrite the variable ci_extras:
See the test hello1.py, and the corresponding pipeline, where I print the content of ci_extras in line 43 and line 43.

[mahendrapaipuri]

Nice find @donion74 . This is not just case of overriding. If we play around the test file hello1.py, we notice that the ci_extras of the first "found" test (presumably by alphabetical order) is applied to all tests. In the pipeline example that @donion74 that posted in previous comment, Simple test should return empty ci_extras but it still returns the ci_extras of HelloArtifacts which is the first prepared test case.

This has to do with test instantiation.

[mahendrapaipuri]

@vkarak I managed to find the problem but not sure about the solution. Lets take a simplified test case:

@rfm.simple_test
class HelloArtifacts(rfm.RegressionTest):
    """only runs in merge requests and stores additional artifacts"""
    valid_systems = ['*']
    valid_prog_environs = ['*']
    sourcepath = 'hello.c'
    ci_extras['gitlab'] = {"rules": [{"if": "$CI_MERGE_REQUEST_ID"}],
                           'artifacts': {"paths": ['.gitlab-ci.yml']}}

    @sanity_function
    def assert_hello(self):
        print(self.ci_extras['gitlab'])
        return sn.assert_found(r'Hello, World\!', self.stdout)

@rfm.simple_test
class Simple(rfm.RegressionTest):
    """always runs"""
    valid_systems = ['*']
    valid_prog_environs = ['*']
    sourcepath = 'hello.c'
    desc = 'Simple'

    @sanity_function
    def assert_true(self):
        print(self.ci_extras)
        return True

This would produce a pipeline.yml as follows:

cache:
  key: ${CI_COMMIT_REF_SLUG}
  paths:
  - rfm-stage/${CI_COMMIT_SHORT_SHA}
stages:
- rfm-stage-0
Simple:
  stage: rfm-stage-0
  script:
  - reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA}  -c .  --report-file=Simple-report.json  --report-junit=Simple-report.xml -vvvvv -n '^Simple$' -r
  artifacts:
    paths:
    - Simple-report.json
    - .gitlab-ci.yml
  needs: []
  rules:
  - if: $CI_MERGE_REQUEST_ID
HelloArtifacts:
  stage: rfm-stage-0
  script:
  - reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA}  -c .  --report-file=HelloArtifacts-report.json  --report-junit=HelloArtifacts-report.xml -vvvvv -n '^HelloArtifacts$' -r
  artifacts:
    paths:
    - HelloArtifacts-report.json
    - .gitlab-ci.yml
  needs: []
  rules:
  - if: $CI_MERGE_REQUEST_ID

This is the buggy behaviour where ci_extras in Simple test are being populated from HelloArtifacts. However, if we change the way we set ci_extras to

 ci_extras = {''gitlab'': {"rules": [{"if": "$CI_MERGE_REQUEST_ID"}],
                           'artifacts': {"paths": ['.gitlab-ci.yml']}}}

we get correct behaviour giving us following pipeline.yml

cache:
  key: ${CI_COMMIT_REF_SLUG}
  paths:
  - rfm-stage/${CI_COMMIT_SHORT_SHA}
stages:
- rfm-stage-0
Simple:
  stage: rfm-stage-0
  script:
  - reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA}  -c .  --report-file=Simple-report.json  --report-junit=Simple-report.xml -vvvvv -n '^Simple$' -r
  artifacts:
    paths:
    - Simple-report.json
  needs: []
HelloArtifacts:
  stage: rfm-stage-0
  script:
  - reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA}  -c .  --report-file=HelloArtifacts-report.json  --report-junit=HelloArtifacts-report.xml -vvvvv -n '^HelloArtifacts$' -r
  artifacts:
    paths:
    - HelloArtifacts-report.json
    - .gitlab-ci.yml
  needs: []
  rules:
  - if: $CI_MERGE_REQUEST_ID

@vkarak , I hope that can give you an idea where to look at!!

[vkarak]

@donion74 I accidentally deleted my previous comment. As @mahendrapaipuri said, If you instead set the ci_extras completely and not just the 'gitlab' element, it should work. I am still investigating why this is happening and whether it is a corner case bug of the variables mechanism. I have fixed the PR to avoid this from happening by setting the default to None, so that your original example will be rejected.

[vkarak]

It is indeed a bug introduced in 4.0. See #2848. I will revert this PR's implementation using {} as the default value and @donion74 meanwhile update your tests to set the ci_extras as a whole until this bug is fixed. Good catch!

[donion74]

great, thanks. It works as expected, there is only some minor issue (I am not sure whether it is related to the changes that you have done): The jobs run successfully, but fails due to a missing directory, see https://gitlab.com/latticeqcd/reframe-debug/-/pipelines/828496576. If I run the tests locally, it does not occur.

[mahendrapaipuri]

@vkarak @donion74 Seems like this regression is coming from this commit. In the CI tests, we are explicitly setting the --report-file with relative path. The basedir is being calculated on relative path and hence basedir is a empty string. Wrapping report_file inside os.path.abspath should fix the problem.

[vkarak]

@mahendrapaipuri Good catch, would you mind opening an issue? The --report-file should work with relative paths...

[mahendrapaipuri]

@vkarak Issue #2849

[mahendrapaipuri]

@vkarak @donion74 I was meaning to ask you if it is worth to include this new ci_extras in the ReFrame config file. So, instead of setting a default value within the class, we get it from the config file.

The use case is that users can define a global CI config in the ReFrame config and override it for specific tests within the test. This is especially useful for users who wish to use same CI config for all tests as they will not need to set the config for "each" test.

[donion74]

This would be very useful. The initial reason I came up with that was to run the Gitlab CI only in merge request pipelines, so I would need to add the same

  rules:
  - if: $CI_MERGE_REQUEST_ID

in all the tests. Your idea would avoid duplicate code.

[vkarak]

@mahendrapaipuri I suggest opening an issue about that. There is also an old one that requests essentially this for any test variable #1500, which we could reconsider.