A basic CI asv check would be useful

[kandersolar]

Is your feature request related to a problem? Please describe.
Currently there's no automated check to ensure that changes to the benchmarks are valid. For example in #1443 and #1445 I think currently the best way to be sure that the new benchmarks are valid (i.e. they won't error when being run in the nightly job) is to checkout the PR locally and try it out manually. It would be nice if this was done automatically somehow.

An automated benchmark check would also prevent us from forgetting to update the benchmarks when we make breaking changes to pvlib itself.

Describe the solution you'd like
A new github actions workflow that builds the asv environments and executes the benchmarks at least once to ensure validity. Note that I'm not suggesting that we actually use the timing results for anything: the goal is to verify that the benchmarks execute without error, not to detect performance regressions. The latter will still be the nightly VM's responsibility.

Describe alternatives you've considered
Running the benchmarks in earnest for PRs would also solve this, but that is still a complicated problem that I don't want to take on at this point. I think this small step in that direction makes more sense for now.

Additional context
asv run --quick seems to do what I want (ref):

Do a "quick" run, where each benchmark function is run only once. This is useful to find basic errors in the benchmark functions faster. The results are unlikely to be useful, and thus are not saved.

--strict is probably also useful here, although see airspeed-velocity/asv#1199

[mikofski]

perhaps config the "ASV-build" action to only occur for release tags instead of every PR commit? that might be enough

[roger-lcc]

I have tried to finish this task these days. But there is a problem I have met.
When I ran asv comman in workflow, CI didn't fail(benchmarks failed). And I have make a reference to the code-checks in pandas. https://github.com/pandas-dev/pandas/blob/main/.github/workflows/code-checks.yml. So I want to konw how CI exactly catch the failure and also the reason why I failed.
Thanks!

[kandersolar]

@roger-lcc are your workflow .yml file and the corresponding github actions log publicly available? Can you link them here?

[roger-lcc]

Here are my workflow.yml.https://github.com/roger-lcc/test/blob/main/.github/workflows/main.yml
And now this problem has been solved.

But there is a problem I have met. When I ran asv comman in workflow, CI didn't fail(benchmarks failed).

However, there is another problem. It seems that CI can't identify if , else structure in benchmark.

https://github.com/roger-lcc/test/actions/runs/2208939226

And I commented out all solarposition.py. Then I got:
https://github.com/roger-lcc/test/runs/6131827793?check_suite_focus=true

[kandersolar]

Thanks @roger-lcc! I think the version parsing issue is because versioneer determines pvlib's version string by looking at the git tags in the repository, but the checkout action by default doesn't pull tags, so it's returning a version like pvlib-0+untagged.1.g6470fc7 instead of 0.9.1.xyz. Try specifying fetch-depth: 0 like we do here: https://github.com/pvlib/pvlib-python/blob/master/.github/workflows/publish.yml#L16-L19

Something else to consider is that we currently use asv 0.4.2 for the nightly job (see https://github.com/pvlib-benchmarker/pvlib-benchmarks/blob/master/README.md?plain=1#L20), and maybe it doesn't really make a difference here but I think we should be consistent about it.

[roger-lcc]

Thanks @kanderso-nrel! Here is my updated workflow.yml.
https://github.com/roger-lcc/test/blob/main/.github/workflows/main.yml
I have tried specifying fetch-depth: 0. However, it didn't work in my test repo. I think maybe the reason is that my test repo doesn't have the git tags.

Try specifying fetch-depth: 0 like we do here: https://github.com/pvlib/pvlibpython/blob/master/.github/workflows/publish.yml#L16-L19

And I have also used asv 0.4.2 in the workflow.

Something else to consider is that we currently use asv 0.4.2 for the nightly job (see https://github.com/pvlib-benchmarker/pvlib-benchmarks/blob/master/README.md?plain=1#L20), and maybe it doesn't really make a difference here but I think we should be consistent about it.

When I test the workflow, it seems like that the path filter doesn't work. It also runs when I push other path commit. And I'm a little confused.

[kandersolar]

I think maybe the reason is that my test repo doesn't have the git tags.

Yes, you're right. I didn't realize that the test repository wasn't a normal fork of pvlib. I guess you could tag a commit as 0.9.1 just to make the benchmarks run correctly, or just leave them disabled for your test, since they're not really needed to see if the github action works as intended.

When I test the workflow, it seems like that the path filter doesn't work. It also runs when I push other path commit. And I'm a little confused.

I don't immediately see a reason why the path filter wouldn't work. But I don't think we even need a path filter anyway because changes outside of the benchmarks directory can also break the benchmarks, for example if we change a parameter name in a pvlib function, or drop support for python 3.6, etc. So I think it probably makes sense to always run this benchmark job, without filtering on paths.

[roger-lcc]

The workflow worked successfully when I pushed git tag. And there is my workflow.yml and result.
https://github.com/roger-lcc/test/blob/main/.github/workflows/main.yml
https://github.com/roger-lcc/test/actions/runs/2228301572

Yes, you're right. I didn't realize that the test repository wasn't a normal fork of pvlib. I guess you could tag a commit as 0.9.1 just to make the benchmarks run correctly, or just leave them disabled for your test, since they're not really needed to see if the github action works as intended.

And CI failed when I added a wrong benchmark. There is my result.
https://github.com/roger-lcc/test/actions/runs/2228331992

[kandersolar]

Thanks @roger-lcc, this looks great! A couple comments:

• Would it make sense to use --show-stderr to show information about failed benchmarks? Right now it just prints the name of the failed benchmark, which is fine, but some more information (like an error traceback or something) would be nice.
• I think it makes sense to omit --python=same so that the environments specified in the configuration file are used. That would make it so that you don't have to pip install ephem and numba in the workflow file too.

Is it time to open a PR containing that workflow file? It would be good to get this in place before merging #1443 and #1445.

[roger-lcc]

I have added --show-stderr in workflow.yml. But it didn't show error traceback in summary. I am a little confused with the sed command. And I will learn about it later.

• Would it make sense to use --show-stderr to show information about failed benchmarks? Right now it just prints the name of the failed benchmark, which is fine, but some more information (like an error traceback or something) would be nice.

I have tried to omit --python=same . However, it will shows Unknown branch master in configuration. And I didn't find something about the branch in asv.conf.

• I think it makes sense to omit --python=same so that the environments specified in the configuration file are used. That would make it so that you don't have to pip install ephem and numba in the workflow file too.