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

[feat] Add CPU vendor and model name to topology files #3107

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

jack-morrison
Copy link
Contributor

@jack-morrison jack-morrison commented Feb 4, 2024

By having the CPU vendor identifier available, test authors can use "GeniuneIntel", "AuthenticAMD", etc. as a logic condition where high-level characterization is acceptable. There are cases where this may actually be more useful than the particular CPU model name.

Fixes ##2995

@jenkins-cscs
Copy link
Collaborator

Can I test this patch?

@jack-morrison
Copy link
Contributor Author

Got some tests to fix up here, it seems.

@teojgo
Copy link
Contributor

teojgo commented Feb 5, 2024

@jack-morrison since you are mentioning the model name of the CPU, do you think it could be added too?

@jack-morrison
Copy link
Contributor Author

jack-morrison commented Feb 5, 2024

@jack-morrison since you are mentioning the model name of the CPU, do you think it could be added too?

I think that's already available in the topology files as arch (relevant archspec docs), on the line immediately above this change.

Edit: I guess it's the microarchitecture name that's already presented as arch. This what I was intending to refer to. I'm happy to add any (or all) other attributes from archspec.cpu though if that's desirable.

@vkarak
Copy link
Contributor

vkarak commented Feb 6, 2024

@jack-morrison The CI fails because you need to update the configuration schema as well:

"properties": {
"arch": {"type": "string"},
"num_cpus": {"type": "number"},
"num_cpus_per_core": {"type": "number"},
"num_cpus_per_socket": {"type": "number"},
"num_sockets": {"type": "number"},
"topology": {"$ref": "#/defs/topology_info"}
},

@teojgo
Copy link
Contributor

teojgo commented Feb 6, 2024

@jack-morrison since you are mentioning the model name of the CPU, do you think it could be added too?

I think that's already available in the topology files as arch (relevant archspec docs), on the line immediately above this change.

Edit: I guess it's the microarchitecture name that's already presented as arch. This what I was intending to refer to. I'm happy to add any (or all) other attributes from archspec.cpu though if that's desirable.

Yeah, it's indeed the microarchitecture, the model name I can retrieve using: archspec.cpu.detect.raw_info_dictionary()['model name'].

@jack-morrison jack-morrison changed the title [feat] Add CPU vendor to topology files [feat] Add CPU vendor and model name to topology files Feb 13, 2024
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ead9f84) 86.64% compared to head (9d122a0) 86.64%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3107   +/-   ##
========================================
  Coverage    86.64%   86.64%           
========================================
  Files           61       61           
  Lines        12051    12051           
========================================
  Hits         10442    10442           
  Misses        1609     1609           

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

reframe/utility/cpuinfo.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Feb 13, 2024

Hello @jack-morrison, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2024-02-13 22:05:27 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants