Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Add benchmarks sps profile test. #130

Merged
merged 9 commits into from
Nov 16, 2020
Merged

Add benchmarks sps profile test. #130

merged 9 commits into from
Nov 16, 2020

Conversation

cdmatters
Copy link
Contributor

@cdmatters cdmatters commented Nov 15, 2020

TL:DR;

This PR involves 2 fixes.

  • 1st a fixup to check_nethack_speed to ensure it runs. Sadly as its not hooked into any automated tests I think it just got a bit out of sync with the API.
  • 2nd is an introduction of a profiling test using pytest-benchmark. I think this set up will allow us to effectively and (relatively) reliably measure SPS for our code changes. The output of such a pytest-benchmark is very also informative - note the OPS column gives us the mean kilo-steps-per-second.

Example Output

master

Why use the benchmark instead of the former script**?

Well it seemed to me that the former script didn't really measure the steps per second of a single environment, rather the maximum throughput one could get running all the environments in suprocesses and putting the number of steps in a shared queue (which could be still useful!).

To illustrate my point consider the following screenshot, in which you choose 1, 2, 5, and (default) 10 environments to use. Clearly there's an communication overhead that becomes the dominant factor at 50k steps per second, which is misleading given each of the 10 environments can reach about 10k steps a second.

Screenshot 2020-11-15 at 15 46 03

As a result I propose using the pytest-benchmark to run performance tests, and perhaps rename check_nethack_speed to something more descriptive? I'm open to suggestions.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 15, 2020
@rockt
Copy link
Contributor

rockt commented Nov 16, 2020

That's great and really useful. Thanks @condnsdmatters !

@heiner
Copy link
Contributor

heiner commented Nov 16, 2020

Black still not happy? (I can really recommend auto-running black on saving in your editor!)

Otherwise LGTM. This script is old and was used before we had libnethack. In a sense process overhead was an even worse problem at the time but perhaps we cared less :)

@edran
Copy link
Contributor

edran commented Nov 16, 2020

Black still not happy? (I can really recommend auto-running black on saving in your editor!)

@condnsdmatters In the nle repo's root, run $ pre-commit install :)

Copy link
Contributor

@edran edran left a comment

Choose a reason for hiding this comment

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

+1 on using pytest-benchmark. The added test looks good.

@cdmatters cdmatters force-pushed the eric/speed-test branch 5 times, most recently from 3278504 to 045f7ce Compare November 16, 2020 12:04
@cdmatters cdmatters merged commit 6a4e655 into master Nov 16, 2020
@cdmatters cdmatters deleted the eric/speed-test branch November 16, 2020 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants