Skip to content
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

Support running benchmarks on multiple long-standing branches of a repository #430

Merged
merged 6 commits into from Apr 27, 2023

Conversation

punchagan
Copy link
Contributor

Previously, we only ran the benchmarks on the default branch of a repository, apart from the pull requests. This PR adds support to run benchmarks on multiple branches of a repository, not just the default branch.

NOTE: This also includes the change in #428. That PR can either be merged before this one, or simply closed, in favor of this.

Closes #426

@punchagan punchagan force-pushed the multi-branch branch 3 times, most recently from e78519b to 20aeb70 Compare April 18, 2023 08:02
Copy link
Contributor

@ElectreAAS ElectreAAS left a comment

Choose a reason for hiding this comment

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

Overall the code seems good (bar my minor comments), but I am not able to properly test that it works.
I tried adding

{ "name": "ElectreAAS/merlin",
  "branches": [ "master", "mock-without-label" ]
}

to environment/development.conf, and adding commits to my mock repo, but the only thing that seems to run is the master branch.
What would be a better way to test it?

pipeline/lib/pipeline.ml Outdated Show resolved Hide resolved
@punchagan
Copy link
Contributor Author

Overall the code seems good (bar my minor comments), but I am not able to properly test that it works. I tried adding

{ "name": "ElectreAAS/merlin",
  "branches": [ "master", "mock-without-label" ]
}

to environment/development.conf, and adding commits to my mock repo, but the only thing that seems to run is the master branch. What would be a better way to test it?

This is how I tested it, too. Did you restart the containers after changing the development.conf file? Also, we ignore commits which are older than 2 weeks old, when running benchmarks (See Pipeline.filter_stale_repositories). I can't think of any other reason why you're unable to test it.

@punchagan
Copy link
Contributor Author

punchagan commented Apr 21, 2023

Overall the code seems good (bar my minor comments), but I am not able to properly test that it works. I tried adding

{ "name": "ElectreAAS/merlin",
  "branches": [ "master", "mock-without-label" ]
}

to environment/development.conf, and adding commits to my mock repo, but the only thing that seems to run is the master branch. What would be a better way to test it?

This is how I tested it, too. Did you restart the containers after changing the development.conf file? Also, we ignore commits which are older than 2 weeks old, when running benchmarks (See Pipeline.filter_stale_repositories). I can't think of any other reason why you're unable to test it.

EDIT: I see that your commits on the branch are just about 2 weeks old, but also I see there's a "Pending" status from your local current-bench deployment on the latest commit on mock-without-label. So, not sure what exactly happened here, but might be worth looking at the logs again?

@ElectreAAS
Copy link
Contributor

Thanks for the verification. With more recent commits everything runs fine indeed!
I'll consider this PR good to go when the other conversation is resolved :)

Previously, we filtered out the branches which don't have the required label,
before starting the benchmarking run. But, this meant that the metadata for the
branch is already saved to the DB and displayed in the UI. This commit changes
the filtering to be run before the metadata for the benchmark run is setup.
This would be useful for benchmarking more than one long running branches of a
repository, for instance, the `5.0` and `trunk` branches of ocaml/ocaml.
@ElectreAAS
Copy link
Contributor

There was a CI failure that vanished when I rebuilt it, I have no clue what caused it.
I'll consider this good to go.

@ElectreAAS ElectreAAS merged commit a2bc44e into ocurrent:main Apr 27, 2023
1 check passed
@punchagan
Copy link
Contributor Author

Thanks for the careful review and merge, @ElectreAAS !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support specifying additional branches to run benchmarks on
2 participants