-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add HardwareResourcesDescription class #47175
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47175/43416 |
A new Pull Request was created by @makortel for master. It involves the following packages:
@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
+1 Size: This PR adds an extra 40KB to repository Comparison SummarySummary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discussion with @Dr15Jones
- Create a new package
FWCore/AbstractServices
and moveResourceInformation
base class there- The new package would depend on
DataFormats/Provenance
- Eventually all other service base classes would be moved to the new package
- The new package would depend on
- Change
ResourceInformation::acceleratorTypes()
to hold the contents ofprocess.options.accelerators
as strings- Despite being "stringly typing", it avoids having to update the code in
ResourceInformationService
every time we add (or test) a new accelerator type
- Despite being "stringly typing", it avoids having to update the code in
process.options.accelerators
schema should be enforced- Best schema that can be enforced would be
\w+-\w+
accompanied with documentation<type>-<vendor>
, with the exception ofcpu
- Check the schema in both python and in ResourceInformationService
- Python should guarantee the
process.options.accelerators
is sorted- Downstream could then use binary search
- Best schema that can be enforced would be
- Add a function
hasGpuNvidia()
toResourceInformation
875fcb4
to
36822c9
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47175/43468 |
This class is inteded to deliver the resource information from ResourceInformation service to ProcessConfiguration.
36822c9
to
8cb6597
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47175/43478 |
Pull request #47175 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again. |
@cmsbuild, please test |
+1 Size: This PR adds an extra 40KB to repository Comparison SummarySummary:
|
I noticed a comparison differences in workflow 17034.0 that must be spurious (as this PR only adds code that is not used outside unit tests). I opened an issue for that #47211 . |
+core |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @mandrenguyen, @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR adds a
HardwareResourcesDescription
class to hold the "hardware provenance" information (and thus is part of #30044). Given the plan to add the information toProcessConfiguration
by replacing the so-far-pratically-unusedcmssw/DataFormats/Provenance/interface/ProcessConfiguration.h
Line 58 in 75451d5
with this information, and to avoid any backwards or forwards compatibility issues, the
HardwareResourcesDescription
will be serialized to a string. For that purpose this PR adds a "compact string serializer" that serializes a list ofstd::string
s and containers ofstd::string
s into one string. A compact representation is achieved by using non-printable ASCII characters as the delimiters.A following PR will replace the
PassID
with theHardwareResourcesDescription
in theProcessConfiguration
interface.Resolves cms-sw/framework-team#1172
PR validation:
Added unit tests work.