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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Mar 23, 2024
Merged

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

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.

nathanielsimard
nathanielsimard reviewed Mar 20, 2024
View reviewed changes
backend-comparison/src/persistence/system_info.rs Outdated
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.

backend-comparison/src/persistence/system_info.rs Outdated
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?

@codecov Codecov
Copy link

codecov bot commented Mar 20, 2024 •
edited
Loading

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.

nathanielsimard
nathanielsimard approved these changes Mar 22, 2024
View reviewed changes
backend-comparison/src/persistence/system_info.rs
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
Reviewers

@nathanielsimard nathanielsimard nathanielsimard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@syl20bnr @nathanielsimard