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

Workflow run benchmarks #6

Merged
merged 11 commits into from
Aug 9, 2022
Merged

Conversation

xspronken
Copy link
Collaborator

@xspronken xspronken commented Aug 3, 2022

PR containing the first part of the github workflow.
This workflow runs the benchmarks for add and matmul and stores the result in an artefact.
The workflow will not work yet since the previous PR hasn´t been merged yet.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Left a few small suggestions for improvements, but looks good.

.github/workflows/run_benchmarks.yml Outdated Show resolved Hide resolved
.github/workflows/run_benchmarks.yml Show resolved Hide resolved
.github/workflows/run_benchmarks.yml Show resolved Hide resolved
@hodgestar
Copy link
Contributor

Looking at the benchmark JSON:

  "group": "Add-2-dense-numpy",
  "name": "test_add[2-dense-numpy]",
  "fullname": "test_linear_algebra.py::test_add[2-dense-numpy]",
  "params": {
    "size": 2,
    "density": "dense",
    "dtype": "numpy"
  },
  "param": "2-dense-numpy",

These groups, names, etc are going to be tricky to adjust later, so they should be conceptually clean, not tailored to the current use cases. The name, fullname, params, and param all look sensible. I'm less sure about group.

To me group is defined by "all of the benchmarks in the same group perform the same operation, possible with different parameter". If that is the right definition, then the group names should be things like "add" or maybe "simple-add" or "add-two" (where "two" here refers to the number of things being added, not the size).

I'm fine with this being address in a later PR, but let's at least create an issue on this repo for it now, and keep in mind that we will likely have to throw away or migrate all the benchmark JSON files later if we adjust the groups.

Note that there might be multiple tests that populate the same group (e.g. two different implementations might require rather different implementations). E.g. We could imagine test_add_gpu.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

I left a long comment about the "group" that each result belongs to, but I'm happy for an issue to be opened to discuss that and for this to be merged now.

@xspronken
Copy link
Collaborator Author

Regarding the group I will create an issue to discuss it.

@hodgestar hodgestar merged commit 826c759 into qutip:master Aug 9, 2022
@xspronken xspronken deleted the workflow_run_benchmarks branch August 9, 2022 12:55
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.

None yet

3 participants