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

Add automatic benchmarking #1062

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Feb 6, 2025

Add automatic benchmarking to FLORIS

The PR will supercede #1060 (which itself superceded #992) and seeks to avoid issues running some of the github actions from a fork and further makes the changes direct to main to make sure the changes to gh-pages work together. This shouldn't be an issue going forward since the action is specified to run on develop at a specific time and is not connected to pulls or pushes.

Adds automatic code benchmarking to FLORIS. Proposed solution is to use pytest-benchmark to implement set timing tests:

https://pytest-benchmark.readthedocs.io/en/latest/

https://github.com/ionelmc/pytest-benchmark

And then try to schedule some semi-daily execution of these tests with logged performance checks so we can track changes over time. Here focused on:

https://github.com/benchmark-action/github-action-benchmark

To this end I added a first test to the tests folder including benchmarking to the tests/ folder and confirm the command line:

pytest floris_benchmark_test.py

This first version includes only a few tests with the idea with once the framework is up and running more possible to add future improvements. This PR is a bit in the way of other PRs so descoping a bit to get it done.

@paulf81 paulf81 added the enhancement An improvement of an existing feature label Feb 6, 2025
@paulf81 paulf81 requested review from rafmudaf and misi9170 February 6, 2025 22:13
@paulf81 paulf81 self-assigned this Feb 6, 2025
@paulf81 paulf81 mentioned this pull request Feb 6, 2025
3 tasks
Copy link
Collaborator

@rafmudaf rafmudaf left a comment

Choose a reason for hiding this comment

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

Do you have an example of the results generated by this? Also, how will it be accessed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to add another input file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have a small chicken-egg thing with default, plan it to move to that when it catches up to main

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this would ultimately go into the docs, you might consider combining this whole job in the deploy-pages workflow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know, that would work! Nice idea, @misi9170 ok with you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah no issues for me

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 6, 2025

Do you have an example of the results generated by this? Also, how will it be accessed?

Yes, it makes a nice interactive chart, I think we have to do this first merge for it to exist right, then I can do a second PR linking it into the docs

@rafmudaf
Copy link
Collaborator

rafmudaf commented Feb 6, 2025

Is it intentional to merge into main instead of develop or was that an accident?

@rafmudaf
Copy link
Collaborator

rafmudaf commented Feb 6, 2025

Do you have an example of the results generated by this? Also, how will it be accessed?

Yes, it makes a nice interactive chart, I think we have to do this first merge for it to exist right, then I can do a second PR linking it into the docs

If you have it locally, a screenshot would be nice. It's just tough to review not knowing what it produces.

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 6, 2025

Is it intentional to merge into main instead of develop or was that an accident?

Intentional, it'll be a process pointed at main, which now reminds me why I can't combine it with the publish pages, which points at develop. It has to do with the fact that it can run from a fork, so only pushes to main work as a trigger

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 6, 2025

Do you have an example of the results generated by this? Also, how will it be accessed?

Yes, it makes a nice interactive chart, I think we have to do this first merge for it to exist right, then I can do a second PR linking it into the docs

If you have it locally, a screenshot would be nice. It's just tough to review not knowing what it produces.

https://nrel.github.io/floris/dev/bench/

@paulf81 paulf81 changed the base branch from main to develop February 6, 2025 23:18
@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 6, 2025

@rafmudaf , ok @misi9170 and I talked and made some changes based on your comments and discussing here:

  1. Back to targeting develop
  2. Merging the action into the deploy pages action for simplicity
  3. Still need to deploy pages to sub-dir or else it will delete the history of benchmark timings
  4. Now that we're back to develop, could use default and get rid of the extra input file
  5. I think it's ready now for review

@paulf81 paulf81 requested a review from rafmudaf February 6, 2025 23:30
destination_dir: docs # Publishes to the docs folder


jobs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work? I would have guessed you can't have two "job" blocks

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rafmudaf
Copy link
Collaborator

rafmudaf commented Feb 6, 2025

@paulf81 Some things that would help tidy this up and make a review easier are:

  • Include it where you want it to link in the documentation site
  • Update the PR description with some information on how this works. For example:
    • Is there anything remarkable about the pytest-benchmark tool?
    • A brief description of what it does would be helpful
    • Did you have to configure it in any way?
    • How are the plots generated and are those customizable?
  • What test cases did you include and why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants