-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat] Add configurable options to generate CI #2420
base: master
Are you sure you want to change the base?
[feat] Add configurable options to generate CI #2420
Conversation
Can I test this patch? |
Codecov Report
@@ Coverage Diff @@
## master #2420 +/- ##
=======================================
Coverage 85.66% 85.67%
=======================================
Files 56 56
Lines 10466 10472 +6
=======================================
+ Hits 8966 8972 +6
Misses 1500 1500
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mahendrapaipuri for the PR. I think we can generalise what you are trying to do here and support arbitrary additions to the child pipeline. We would also need a unit test to check that these additional pieces are emitted correctly.
"generate-ci/after_script": ["echo 'noop'"], | ||
"generate-ci/before_script": ["echo 'noop'"], | ||
"generate-ci/artifacts": [], | ||
"generate-ci/artifacts_expiry": "30 days", | ||
"generate-ci/target_systems": ["*"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't those too specific to Gitlab? I had also this a bit differently in mind. That we allowed the users to specific any structure, which we could then combine with reframe's generated one. What about something like this?
"generate-ci/after_script": ["echo 'noop'"], | |
"generate-ci/before_script": ["echo 'noop'"], | |
"generate-ci/artifacts": [], | |
"generate-ci/artifacts_expiry": "30 days", | |
"generate-ci/target_systems": ["*"], | |
"ci-integration/backend": "gitlab", | |
"ci-integration/pipeline_extras": {} |
And the users could write:
"ci-integration": [
{
"target_system": ["*"],
"backend": "gitlab",
"pipeline": {
"after_script": ["echo noop"],
"before_script": ["echo noop"],
"artifacts": ["output"],
"artifacts_expiry": "30d"
}
}
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree this is too specific for Gitlab. I like your idea and it gives us more freedom to add other backends in the future. I was thinking more along using a template and then merging it with the ReFrame generated one. The rationale is GitLab CI provides ton of variables for CI. How many of them can we actually support using the current approach? Also, we need to write logic to get the YAML file syntax right. I mean placing these keywords in proper hierarchy. With a template approach, we delegate this to the user to provide a template with correct syntax and keywords. For instance, ReFrame produces a child YAML file something like this:
rfm-stage-0:
script:
- <reframe test 0>
artifacts:
paths:
- test-0-report.json
rfm-stage-1:
script:
- <reframe test 1>
artifacts:
paths:
- test-1-report.json
A template can be something like this:
stages:
- test
- post-test
rfm-stage:
stage: test
before_script:
- echo "Pretest commands"
after_script:
- echo "Posttest commands"
artifacts:
paths:
- my_artifact
expire_in: 1 week
post-processing:
stage: post-test
script:
- echo "Postprocessing commands"
artifacts:
paths:
- my_postprocess_artifact
expire_in: 2 week
The contents of rfm-stage
block will be merged. And the rest of the template will be appended to generated pipeline. This way we give more freedom to the users on how they want to organise their CI tests. A post processing job in the above example can be to merge all junit xml files from all tests to get one global file which can be used in GitLab CI badges. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense what you propose here. Is your template a template for the .gitlab.yaml
or a template for the child pipeline that ReFrame generates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I am proposing here is the full .gitlab.yml
template which is superset of child pipeline template. It opens up all the possibilities for the users on how they want to organise their tests. I will try to do a draft implementation and then we can iterate over it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, I'm in favor of the idea, but I need to understand a bit the workflow. What is the template that reframe reads and what does it generate exactly? That'd help before starting the implementation I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take the OSU test example to see how this will work.
Current ReFrame CI generator
If we generate CI child pipeline for this test, we will end up with the following yaml file:
cache:
key: ${CI_COMMIT_REF_SLUG}
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA}
stages:
- rfm-stage-0
- rfm-stage-1
- rfm-stage-2
OSUDownloadTest:
stage: rfm-stage-0
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUDownloadTest-report.json --report-junit=OSUDownloadTest-report.xml -n '^OSUDownloadTest$' -r
artifacts:
paths:
- OSUDownloadTest-report.json
needs: []
OSUBuildTest:
stage: rfm-stage-1
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUBuildTest-report.json --restore-session=OSUDownloadTest-report.json --report-junit=OSUBuildTest-report.xml -n '^OSUBuildTest$' -r
artifacts:
paths:
- OSUBuildTest-report.json
needs:
- OSUDownloadTest
OSUAllreduceTest_16:
stage: rfm-stage-2
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_16-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_16-report.xml -n '^OSUAllreduceTest_16$' -r
artifacts:
paths:
- OSUAllreduceTest_16-report.json
needs:
- OSUBuildTest
OSUAllreduceTest_8:
stage: rfm-stage-2
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_8-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_8-report.xml -n '^OSUAllreduceTest_8$' -r
artifacts:
paths:
- OSUAllreduceTest_8-report.json
needs:
- OSUBuildTest
OSUAllreduceTest_4:
stage: rfm-stage-2
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_4-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_4-report.xml -n '^OSUAllreduceTest_4$' -r
artifacts:
paths:
- OSUAllreduceTest_4-report.json
needs:
- OSUBuildTest
OSUAllreduceTest_2:
stage: rfm-stage-2
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_2-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_2-report.xml -n '^OSUAllreduceTest_2$' -r
artifacts:
paths:
- OSUAllreduceTest_2-report.json
needs:
- OSUBuildTest
OSUBandwidthTest:
stage: rfm-stage-2
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUBandwidthTest-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUBandwidthTest-report.xml -n '^OSUBandwidthTest$' -r
artifacts:
paths:
- OSUBandwidthTest-report.json
needs:
- OSUBuildTest
OSULatencyTest:
stage: rfm-stage-2
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSULatencyTest-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSULatencyTest-report.xml -n '^OSULatencyTest$' -r
artifacts:
paths:
- OSULatencyTest-report.json
needs:
- OSUBuildTest
Our aim here is to provide arbitrary configurability for this generated yaml file.
Proposal
Suppose the user wants to configure his/her pipeline with keywords before_script
, after_script
and even additional stages besides the one generated by ReFrame test. One obvious question here is why can't we write a ReFrame test to generate that "additional" stage as well? Back then it was not straight-forward to write a dependent test that has dependency on parameterised tests. (Here is a brief Slack conversation). I guess now it is possible with the fixtures. So the user can provide a "template" as follows:
image: registry.gitlab.com/myproject/myimage
stages:
- rfm-stage-0
- rfm-stage-1
- rfm-stage-2
template-stage-0:
stage: rfm-stage-0
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
template-stage-1:
stage: rfm-stage-1
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
template-stage-2:
stage: rfm-stage-2
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
The important thing here is we define template per stage. We know the number of stages that will be generated a priori and naming convention is pretty straight-forward here rfm-stage-[\d+]
. The user defines this template per stage and this will be merged to the main yaml file generated by the ReFrame. The name of the stage per se is not really important as we will merge this template based on the stage
keyword. The user can pass this file path via CLI flag or environment variable. In ci.py
we read this file and merge it with the one generated by the ReFrame which spits out a generated file as follows for the current example:
image: registry.gitlab.com/myproject/myimage
cache:
key: ${CI_COMMIT_REF_SLUG}
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA}
stages:
- rfm-stage-0
- rfm-stage-1
- rfm-stage-2
OSUDownloadTest:
stage: rfm-stage-0
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUDownloadTest-report.json --report-junit=OSUDownloadTest-report.xml -n '^OSUDownloadTest$' -r
artifacts:
paths:
- OSUDownloadTest-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs: []
OSUBuildTest:
stage: rfm-stage-1
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUBuildTest-report.json --restore-session=OSUDownloadTest-report.json --report-junit=OSUBuildTest-report.xml -n '^OSUBuildTest$' -r
artifacts:
paths:
- OSUBuildTest-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs:
- OSUDownloadTest
OSUAllreduceTest_16:
stage: rfm-stage-2
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_16-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_16-report.xml -n '^OSUAllreduceTest_16$' -r
artifacts:
paths:
- OSUAllreduceTest_16-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs:
- OSUBuildTest
OSUAllreduceTest_8:
stage: rfm-stage-2
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_8-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_8-report.xml -n '^OSUAllreduceTest_8$' -r
artifacts:
paths:
- OSUAllreduceTest_8-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs:
- OSUBuildTest
OSUAllreduceTest_4:
stage: rfm-stage-2
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_4-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_4-report.xml -n '^OSUAllreduceTest_4$' -r
artifacts:
paths:
- OSUAllreduceTest_4-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs:
- OSUBuildTest
OSUAllreduceTest_2:
stage: rfm-stage-2
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUAllreduceTest_2-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUAllreduceTest_2-report.xml -n '^OSUAllreduceTest_2$' -r
artifacts:
paths:
- OSUAllreduceTest_2-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs:
- OSUBuildTest
OSUBandwidthTest:
stage: rfm-stage-2
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSUBandwidthTest-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSUBandwidthTest-report.xml -n '^OSUBandwidthTest$' -r
artifacts:
paths:
- OSUBandwidthTest-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs:
- OSUBuildTest
OSULatencyTest:
stage: rfm-stage-2
before_script:
- source $HOME/.bashrc
script:
- reframe --prefix=rfm-stage/${CI_COMMIT_SHORT_SHA} -C reframe_upstream_config.py -c reframe_osutest.py -R --report-file=OSULatencyTest-report.json --restore-session=OSUBuildTest-report.json --report-junit=OSULatencyTest-report.xml -n '^OSULatencyTest$' -r
artifacts:
paths:
- OSULatencyTest-report.json
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
needs:
- OSUBuildTest
If the user passed yaml file has syntax errors, the generated file will inherit them too. So it is the responsibility of the user to get the template file right. This way the user can arbitrarily configure the CI pipelines with all the options provided by the Gitlab. This also helps the developers to add support for other backends easily with minimal configuration implemented in ReFrame and delegating the advanced config options to user via templates.
I was thinking along these lines and of course we can improve this idea further. Please let me know if the workflow is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mahendrapaipuri, I finally had some time to put my thoughts together on this. Your idea is in the right direction, but we could tweak it a bit:
- Instead of passing an option to ReFrame with the templates, I would rather have a file at the top-level directory named for example
.rfm-ci.yml
, which reframe would load it if it exists and read the templates. - Regarding the template, I would rather provide a syntax on top of Gitlab's CI as follows:
stage-template-x:
match:
stage: rfm-stage-[12]
ci-config:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
build-template:
match:
job: OSUBuildTest.*
ci-config:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
In this file you write CI templates and you request them to either match a job (aka test) or a stage. The framework when generating stage X will check if there is a matching template and if it exists, it will merge its ci-config
part with the generated one. For each job it generates, it will also check if there is a matching template and apply the ci-config
part. These could be stacked, the stage template is applied first and then then the test template. So in the above example, the stage-template-x
will apply to all jobs in stages 1 and 2, whereas the build-template
will apply (additionally) to all jobs starting with OSUBuildTest
.
What do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @vkarak, Sorry for late response, I was away.
I like your template approach and it is more elegant. I have a question on how do we pass this template:
- You suggestion of having a file
.rfm-ci.yml
at root directory is in correct direction. But how do we support if user wants to have a template file per test? There can be use cases where users might want to save different artifiacts for different tests. - One solution that I can think of is that ReFrame looks for CI template in the root directory and also in
src/
of the test. If it finds the template insrc/
that file will take precedence over the one in root directory. - Wait, actually your suggestion of using
match
withjob
keyword can address this issue. The user can define the templates for all the tests here based on job name and we simply match them within ReFrame. If we decide to go in this direction, we cannot usematch
withstage
keyword for obvious reasons as they are not unique across the tests. With this approach we can even support inline file reference usinginclude
like in Spack where users can separate the template files for each test and refer them in the.rfm-ci.yaml
file usinginclude
keyword. We can simplify the template as follows:
OSUBuildTest.*:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
OSUAllreduceTest.*:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
And we directly match the key of the dict with job name in generated CI. With inline can look like this:
include: /path/to/ci/templates/OSU/OSUBuildTest
include: /path/to/ci/templates/OSU/OSUAllreduceTest
How does it sound to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're suggesting to drop the matching for stages and keep only that for tests. I'm fine with that. Also I wouldn't have a problem with your template proposal, assuming that's it's valid yaml syntax to have special characters, such *
and any other regex special characters, in the key name. If not, we could use a template syntax along the lines of my proposal above, where the regex is a value of "normal" key. Also just to check my understanding regarding the include
suggestion: the following snippet
OSUBuildTest.*:
include: /path/to/ci/templates/OSU/OSUBuildTest
OSUAllreduceTest.*:
include: /path/to/ci/templates/OSU/OSUAllreduceTest
will be equivalent to this, assuming the contents of the template files are the same, right?
OSUBuildTest.*:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
OSUAllreduceTest.*:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
I'm fine with this proposal.
@@ -65,9 +77,13 @@ def rfm_command(testcase): | |||
for tc in testcases: | |||
json[f'{tc.check.unique_name}'] = { | |||
'stage': f'rfm-stage-{tc.level}', | |||
'before_script': copy.deepcopy(before_script), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the deepcopy here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I do not remember. Probably it is vestigial as I was trying out few things back then. Probably we can remove it.
artifacts = [os.path.join(prefix, a) | ||
if a in ['perflogs', 'stage', 'output'] | ||
else a for a in artifacts] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably support placeholders in the configuration, so that you can write:
{
'artifacts': ['{CI_PREFIX}/foo.log', ...]
}
And here you do something like this:
pipeline_extras = copy.deepcopy(config.get('ci-integration/0/pipeline_extras'))
try:
artifacts = [ar.format(CI_PREFIX=prefix) for ar in pipeline_extras['artifacts']]
except KeyError:
artifacts = []
else:
pipeline_extras['artifacts'] = artifacts
for tc in testcases:
json[...].update(pipeline_extras)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good for me. Would make this change! Cheers!!
@mahendrapaipuri I was thinking to close this PR since it's inactive for a while. Feel free to reopen it whenever you want! |
Hello @vkarak, really sorry for this outstanding PR. Been really busy with other projects. But I am going to write regression tests for our HPC platform using ReFrame and so I will come back to it soon. I hope to have this PR ready before the end of this year. |
That sounds great! I am opening it again then! |
Codecov ReportBase: 85.66% // Head: 86.27% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2420 +/- ##
==========================================
+ Coverage 85.66% 86.27% +0.60%
==========================================
Files 56 60 +4
Lines 10466 10991 +525
==========================================
+ Hits 8966 9482 +516
- Misses 1500 1509 +9
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 at Codecov. |
Codecov ReportBase: 86.61% // Head: 86.61% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2420 +/- ##
=======================================
Coverage 86.61% 86.61%
=======================================
Files 60 60
Lines 11196 11202 +6
=======================================
+ Hits 9697 9703 +6
Misses 1499 1499
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 at Codecov. |
Closes #2162