[backend-comparison] Add GitHub authentication to burnbench CLI

[syl20bnr]

Checklist

• Confirmed that run-checks all script has been executed.
• Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Issue #1072

Changes

Add a new auth command to burnbench in order to create an access token that will be used to authenticate the user when sharing the benchmark results.

Add a --share argument to the run command to upload the benchmark results. This is a WIP and the actual upload will be added once the server side used the access token to authenticate the user.

Found a crate called github-device-flow to help with implementing the device flow in order to create a user access token.

For now the token is not refreshed when it expires. This can be implemented later.

Currently the GitHub application is a temporary one called Burnbench POC created under my account. Once this is approved and merged we can create the official one in the Tracel org and update the client ID.

Testing

Would be great if you can test the auth flow with the following commands in this order:

First, verify that the CLI errors out if you are not authenticated and the --share argument is passed:

cargo run --bin burnbench -- run --share --benches unary --backends ndarray

Then follow the authentication flow with:

cargo run --bin burnbench -- auth

At last rerun the benchmark with --share argument:

cargo run --bin burnbench -- run --share --benches unary --backends ndarray

Note that the actual upload is not yet implemented.

[laggui]

Followed your instructions and seemed to work as described :)

As expected, the first command fails without auth:

You need to be authenticated to be able to share benchmark results.
Run the command 'burnbench auth' to authenticate.

After authenticating the benchmark completed successfully.

―――――――― Result ―――――――――
  Samples     10
  Mean        232.641ms
  Variance    91.636µs
  Median      231.984ms
  Min         219.470ms
  Max         249.241ms
―――――――――――――――――――――――――

Also noted a minor typo below.

[antimora]

I followed the steps successfully, and here are the results:

     Running benches/unary.rs (/Users/dilshod/Projects/burn/target/release/deps/unary-5a7517d0271fa0ff)

        Timestamp: 1707511316062
        Git Hash: 202247d6e59890b97c2dc2f61fcc9b0665a3e85f
        Benchmarking - unary
―――――――― Result ―――――――――
  Samples     10
  Mean        197.687ms
  Variance    1.002µs
  Median      197.450ms
  Min         196.849ms
  Max         200.591ms
―――――――――――――――――――――――――

Cleanup completed. Benchmark run(s) finished.
Sharing results...
[backend-comparison]%

[antimora]

Also confirming that the permissions are set correctly on my Mac:

[backend-comparison]% ll /Users/dilshod/.cache/burn/burnbench/token.txt
-rw-------  1 dilshod  staff  40 Feb  9 14:41 /Users/dilshod/.cache/burn/burnbench/token.txt

[antimora]

LGTM

[Luni-4]

Fantastic! Thanks a lot!

Two considerations (you can tick them if they are deemed valid):

• Can we replace println and eprintln with log functions?
• Perhaps I'm wrong, but it seems there are some tests that could be deduplicated using intermediate functions. Is this wanted?

[nathanielsimard]

• Can we replace println and eprintln with log functions?

I think println is meant to display a command line message more than logging.

[syl20bnr]

@Luni-4 Not sure about the addition of a log mechanism for now given that we already write the benchmarks to disk in the cache directory. Those prints are just part of the UI in a sense. But maybe it can be useful to add a logger for those later, feel free to open an issue about it.

[Luni-4]

@nathanielsimard @syl20bnr

My idea of logging in a library (in our case, a crate) consists of discriminating among the different events that might happen when an API is called. A println! just prints a message on shell, but it does not provide any additional information about the event. Through log APIs, we can express our attitudes towards that message communicating a precise behaviour to a user. A con is that we have to add another dependency, but it should be a lightweight one.

Anyway, I agree to maintain a println and eprintln, they are just more imprecise in my opinion, not wrong, so we can add logging functions later. Yep, I can open an issue if you want.

[syl20bnr]

Perhaps I'm wrong, but it seems there are some tests that could be deduplicated using intermediate functions. Is this wanted?

Each test tests only one thing and each case is different, we cannot really do much more than the cleanup function that is already made to reduce redundancy. Maybe we could be more fancy with the setup and teardown but that does not seem to be easy in Rust.

[antimora]

@syl20bnr i have used rstest in burn-import. It is pretty cool and easy to use. Check it out if you are interested: https://docs.rs/rstest/latest/rstest/

[syl20bnr]

@antimora unfortunately rstest does not provide a mechanism for setup and teardown. I believe that any attempt in doing it by ourselves will increase the complexity of the tests for little gain.

[Luni-4]

Thanks a lot for your changes! The code has been improved a lot!

Just the last change for me