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

Include a performance test and report in the CI #59

Closed
hagrid67 opened this issue Nov 1, 2023 · 3 comments · Fixed by #88
Closed

Include a performance test and report in the CI #59

hagrid67 opened this issue Nov 1, 2023 · 3 comments · Fixed by #88
Labels
enhancement New feature or request
Milestone

Comments

@hagrid67
Copy link
Contributor

hagrid67 commented Nov 1, 2023

Objectives

  • Introduce 1 or more performance tests, ideally runnable from the CI (possibly optionally)
  • Make the performance measure visible with commits and release versions
  • Provide a table & chart of performance across some historical versions? (Would need to do this as a separate piece of work)

Todos

  • Medium sized environment with (frozen seed) random actions. This will be biased toward lots of collisions and deadlocks.
  • Small / Medium sized environment with a working solution - ideally this will have minimal deadlocks and collisions.
  • Perhaps a breakdown of time into some major categories, eg reset(), step() and build_observation()

Minimal testing requirements

  • This issue is to introduce a new test, perhaps run separately from the functional tests.

Additional context

  • Many competitors and research groups have complained about the performance of the environment.
@hagrid67 hagrid67 added the enhancement New feature or request label Nov 1, 2023
@hagrid67
Copy link
Contributor Author

hagrid67 commented Nov 1, 2023

@fnberta @aiAdrian This one came up in the dev call today. I could do some work on this but would appreciate some input from Fabio on established practice & approaches to CI, and also from Adrian on latest performance work and what should be measured.

@aiAdrian
Copy link
Contributor

aiAdrian commented Nov 1, 2023

Excellent idea. I will think about the problem how we could provide a simple multi agent deadlock avoidance implementation to compare runtimes

@fnberta
Copy link
Contributor

fnberta commented Nov 5, 2023

Hi @hagrid67, thanks for the interesting idea!

I see multiple ways of including such information in the build pipeline:

  1. Define certain benchmarks values and have actual tests that fail if the produced value does not meet the benchmarks. This would mean that a PR could not be merged if a performance regression would be introduced (measured against the pre-defined benchmarks).
  2. Have a performance summary (e.g. an HTML page) that is produced for every commit to main (i.e. every PR that is merged). This page could be deployed somewhere so that it is easily visible. This would not prevent performance regressions to merged but it would make them visible afterwards.
  3. Have a performance summary similar to option 2 but instead of producing it for commits to main, produce it for every commit in a PR. Best case, you would have GitHub bot that reports the results as a comment straight into the PR. Again, this would not prevent regressions to be merged but it would make them immediately visible for every commit and before merging.

I'm sure there are more options, but these are the ones that immediately come to mind! All these options obviously require some kind of stable environment to run reliable benchmarks. We have to check, to what extent GitHub action runners can provide that. In my experience, the performance can vary. But let's evaluate!

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

Successfully merging a pull request may close this issue.

4 participants