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

[backend-comparison] Add system information to benchmark results #1495

Merged
merged 4 commits into from
Mar 23, 2024

Conversation

syl20bnr
Copy link
Member

Pull Request Template

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

#1072

Changes

Add system information using sysinfo crate for the CPUs and wgpu crate for the GPUs.

Comment on lines 36 to 41
.filter(|adapter| {
let info = adapter.get_info();
info.device_type == wgpu::DeviceType::DiscreteGpu
})
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to keep the integrated GPU as well. For instance, newer Macs with M1/2/3 chips use integrated GPUs (powerful ones) with shared memory.

fn enumerate_gpus() -> Vec<String> {
let instance = wgpu::Instance::default();
let adapters: Vec<wgpu::Adapter> = instance
.enumerate_adapters(wgpu::Backends::all())
Copy link
Member

Choose a reason for hiding this comment

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

Is there any duplicates when enumerating all adapters for all backends? Should we select the backend based on the OS here?

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 4.87805% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 85.86%. Comparing base (6feda90) to head (d3fd97c).

Files Patch % Lines
backend-comparison/src/persistence/system_info.rs 0.00% 37 Missing ⚠️
backend-comparison/src/persistence/base.rs 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1495      +/-   ##
==========================================
- Coverage   85.90%   85.86%   -0.05%     
==========================================
  Files         663      664       +1     
  Lines       75447    75487      +40     
==========================================
+ Hits        64816    64818       +2     
- Misses      10631    10669      +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syl20bnr
Copy link
Member Author

I added the integrated GPUs and also the selection algo from burn-wgpu AutoGraphicsApi.

On my intel macbook I get this systeminfo:

  "systemInfo": {
    "cpus": [
      "Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz"
    ],
    "gpus": [
      "AMD Radeon Pro 5300M",
      "Intel(R) UHD Graphics 630"
    ]
  },

@syl20bnr
Copy link
Member Author

We have a regression when using the burn-wgpu crate though, the fusion backend is automatically selected again.

@syl20bnr
Copy link
Member Author

We have a regression when using the burn-wgpu crate though, the fusion backend is automatically selected again.

Fixed by disabling the default features for burn-wgpu in the backend-comparison crate.

@syl20bnr
Copy link
Member Author

Well we end up in the same situation than: #1476

@nathanielsimard so the solution is to remove the fusion default in wgpu ?

@nathanielsimard
Copy link
Member

Well we end up in the same situation than: #1476

@nathanielsimard so the solution is to remove the fusion default in wgpu ?

I think we should just not enable the default features when running the burn-wgpu benchmarks, but I still think we should keep fusion in the default features of burn-wgpu, since it should be automatically enabled by users.

Comment on lines +8 to +19
pub(crate) struct BenchmarkSystemInfo {
cpus: Vec<String>,
gpus: Vec<String>,
}

impl BenchmarkSystemInfo {
pub(crate) fn new() -> Self {
Self {
cpus: BenchmarkSystemInfo::enumerate_cpus(),
gpus: BenchmarkSystemInfo::enumerate_gpus(),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Clean 👌

@syl20bnr syl20bnr merged commit 0adda72 into main Mar 23, 2024
14 of 15 checks passed
@syl20bnr syl20bnr deleted the feat/benchmark-systeminfo branch March 23, 2024 03:24
@syl20bnr
Copy link
Member Author

I merged it but we still have the regression that in the current feature config we cannot enable wgpu alone in the benchmarks. It is not trivial to fix this as mentioned in #1476

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

Successfully merging this pull request may close these issues.

2 participants