Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 performance regression tests in CI #4701
Add performance regression tests in CI #4701
Changes from all commits
969c31f
ac4195d
9f47298
c23002e
6493239
7a55923
46a691b
0d5ed11
d8fb254
c927f87
7a906fe
8661fa1
1bce5ad
b9a88a7
0e9fb3e
6e68c0b
ddca12a
47ab67c
1be1700
f427602
cb336c5
f23f4cf
d91952c
149c300
bf45366
650416c
1f0272e
124d01c
e852dcf
5f5344d
d15d6e0
f596a6f
fad287d
6c40862
79ae666
222ad90
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not really following the logic to return the correct tuple here. What is the result you're trying to return in this function? Why do you need an xor?
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.
You can think of this as a sort on the commits:
This is necessary for the diff functionality to know which version is the standard and which version we want to test for comparison. Between mainline and PR branch, mainline should be the standard for comparison, otherwise the older commit between two PR's which are on the same branch should be the standard for comparison to the new commit.
The xor returns true if exactly one of the commits is mainline, then we can return whichever commit is mainline first. If both or neither are mainline the condition evaluates to false, then we must check for which commit is older, so we move to the else condition in that case which checks for which commit is older.
I will add comments to this function to make this more clear in the code.
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 just seems like we don't need to be guessing at which commit is our "baseline". Doesn't the caller of these tests always know which commit is the baseline versus which one is the altered code? Like, you should always know I want to know the performance change that occurs from "this commit" to "this commit".
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.
Initially, the approach was to have the caller identify "baseline" and "altered" as an environment variable upon invocation, similar to your suggestion. However after some discussion, we decided to solely rely on the commit id to auto detect new/old and in CI, branch/mainline. This means that with solely the commit id's available to identify files we have to figure out which commit ID is "baseline" vs "altered", which this logic attempts to solve. We could add back an environment variable for the caller to set "baseline" and "altered" to make the tool more flexible in its usage but if we base profiles on solely their commit id, the current approach is needed.
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.
Hmmm that's a bummer, I kind of disagree with that approach because this logic looks pretty brittle as it is now. I think it would help for me to know what exactly is our goal here, do we expect this to automatically detect the right commit both when running this locally and running this in CI? Like, is the logic here only for running in CI or is it also trying to work for running the tests locally?
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.
The logic here attempts to cover both, running locally and in CI. Locally, we would want the older of two commits as "baseline" and in CI, we would want main to be "baseline". I agree with you though that the logic is brittle, especially for local testing since I can imagine scenarios where the caller wants to compare two commits that are not on the same branch or set the newer commit as "baseline". To make the usage a bit more extensible, I think we should add an identifier to let the caller decide, so that the artifacts produced are
baseline/commit_hash.test_name.raw
andaltered/commit_hash.test_name.raw
. I would vote to do that in a separate PR though.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.
Yeah then I think you should just make this logic to work in CI. So it would just be like:
I don't think there's much point in commiting complex logic if you're going to rip it out in a different PR.
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.
Given the direction that integv2 went, I think it's important to keep things at least theoretically runnable locally. Also makes putting demos together much easier, etc.
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 mean, if I am running this locally, I would probably create a branch with my changes and want to compare that branch to mainline. So even then, I really would only care about the difference between mainline and my branch.
That being said, the consequences of getting the order wrong are very minor. But I do think this code should be cleaned up in the future.