-
Notifications
You must be signed in to change notification settings - Fork 906
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
Add Github Actions workflow to trigger pipeline performance test #4231
Conversation
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
My initial thought is that this should be triggered not on every push or based on the commit message because someone might want to trigger this after making all the changes so making a random commit just to trigger a pipeline test seems counterintuitive. |
I like the idea! |
Agree on above, I like the idea about label better. How can we trigger it multiple times with label? Perhaps is it possible to trigger the workflow manually with parameter (commit hash/branch name?) |
run: | | ||
echo "Commit message contains 'PERFORMANCE TEST'. Triggering performance test." | ||
|
||
- name: Trigger performance test workflow in test project |
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.
Also I think you can use the checkout
action to clone the performance test repo - https://github.com/actions/checkout
and then call kedro run
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.
Has this been addressed?
Maybe we can have an alternative way to trigger from the test repo itself with a specific branch. |
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
I've changed the trigger method to use a label, and created a "performance" label to test it out. It will trigger when the label is added, and you can repeat the test by removing the label and adding it again or running the job again. Now for a little bit of a philosophical question - My intention was to use commits not only to trigger the test itself but also to set those hook/save/load delay parameters that the test has. If we use a label instead of the commit to trigger the run, how could we set the delays? Maybe we could have both methods available, so you can use the label for a simple test but a commit if you want to manually alter the delays? This is the test branch I'm using, by the way: #4233 |
I really like the label trigger!
@lrcouto How do you set those parameters in the commit? And what are the default now? |
The default right now was no delays. My initial idea was passing the parameter line on the commit message and sending it on the POST payload, same as it's being done with the branch name right now. But I'm looking for alternatives that would work with the label solution. |
I thought of two possible alternatives for this issue that we could use. What I would like to do is allow the user to pass all of the parameters to The first option would be using PR comments to pass parameters and run the workflow. It's easy to use but the disadvantage is that we would pollute the PRs with parameter comments, which makes me not like this option very much. The second would be to have a manually dispatched workflow on the test repo, in which you could go and input the parameters. Then you could use the performance label one for a quick run with all default values, or you could do something with more customization by going to the test repo itself and running it manually. I'm leaning more towards this one. Let me know what you think! |
Maybe for a first iteration just triggering the test as is, is fine and we can create a follow up ticket to explore parameters? #4210 is already meant to check the different runners, so my question is whether it makes sense to also have that as a changing factor for this setup? |
Agree with @merelcht, I think having parameters in commit are also a bit overkill and quite easy to break. I would prefer a sensible default (that are non-zero, otherwise it doesn't shows anything). If there are more needs to extend the parameters, we can always clone the repository and run with source code directly. |
We can leave this as it is for now, and extend it in the future then. |
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.
LGTM! 👍
I do think you could checkout the other repo in this workflow and time and run it so one could see the results in the PR itself instead of triggering the workflow on the repo that contains the project where we would have to look through the Actions tab to see what the results are. |
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.
This looks good to me besides some comments left by @ankatiyar that haven't been addressed.
run: | | ||
echo "Commit message contains 'PERFORMANCE TEST'. Triggering performance test." | ||
|
||
- name: Trigger performance test workflow in test project |
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.
Has this been addressed?
Yeah I think that would be more convenient. I'm rewriting the workflow to work in this way. |
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
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.
LGTM! Thanks @lrcouto 💯
|
||
- name: Set up Python 3.11 | ||
if: github.event.action == 'labeled' && contains(github.event.label.name, 'performance') | ||
uses: actions/setup-python@v4 |
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.
This should be v5 I think there's some deprecations in v4
Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
Description
Adds a workflow to trigger a pipeline performance test on the performance test repo if the user adds the "performance" label to the pull request they wish to test.
The test will run a comparison in elapsed time between the branch with the performance label against the latest kedro release.
Development notes
Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file