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

Change how CUDA runtime and capabilities are defined in the task and Condor #11689

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/python/Utils/Utilities.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import sys
from types import ModuleType, FunctionType
from gc import get_referents
from distutils.version import StrictVersion


def lowerCmsHeaders(headers):
"""
Expand Down Expand Up @@ -295,3 +297,21 @@ def encodeUnicodeToBytesConditional(value, errors="ignore", condition=True):
if condition:
return encodeUnicodeToBytes(value, errors)
return value


def orderVersionList(versionList):
"""
This function will order a list of version-style strings.
The order of precedence digits is from left to right. E.g.:
from: ["2.3.1", "1.2.3", "3.2.1", "1.3.2"]
to: ["1.2.3", "1.3.2", "2.3.1", "3.2.1"]
:param versionList: list of strings
:return: an ordered list; or the initial data if different than list.

NOTE: implementation suggested in:
https://stackoverflow.com/questions/2574080/sorting-a-list-of-dot-separated-numbers-like-software-versions
"""
if not isinstance(versionList, list):
return versionList
versionList.sort(key=StrictVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Other than the good documentation provided with this function, which I admit gives good info, why do we need it. The list.sort method works in place, so executing this same line outside of the function would do just fine. We can still make a NOTE comment providing the information from the docstring though.

return versionList
30 changes: 29 additions & 1 deletion src/python/WMCore/BossAir/Plugins/BasePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from builtins import object, str, bytes
from future.utils import viewvalues

from Utils.Utilities import decodeBytesToUnicode
from Utils.Utilities import decodeBytesToUnicode, orderVersionList
from WMCore.WMException import WMException
from WMCore.WMRuntime.Tools.Scram import ARCH_TO_OS, SCRAM_TO_ARCH

Expand Down Expand Up @@ -181,3 +181,31 @@ def scramArchtoRequiredArch(scramArch=None):
archs = defaultArch

return archs

@staticmethod
def cudaCapabilityToSingleVersion(capabilities=None):
"""
Given a list of CUDA capabilities (with strings in a version style),
finds the smallest version required and convert it to a single integer
for comparison/job matchmaking purposes.
Version conversion formula is: (1000 * major + 10 * medium + minor)
:param capabilities: a list of string versions
:return: an integer with the version value; 0 in case of failure
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the order here ... is 0 with least significance or vise versa?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect the job matchmaking to be available_version >= required_version, so 0 here would mean that any cuda capability is supported by the job. I am not sure this answers your question, otherwise I might have not understood your question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Partially it does.
What bothers me here, is that default value is again 0 and it overlaps with the value returned in case of an error/failure. Which would be a valid case if 0 was never to be reached in the matchmaking condition. But since it is included, then it would trigger similar behavior in failure and by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can return from a version to integer conversion or from an error. However, this code will only be executed if GPUs are required, and if they are, then there are validated GPU parameters and the version is enforced to be something like r"^\d+\.\d+(\.\d+)?$". IF a version provided is 0.0, then yes, we get a "valid" 0 single version, which we could get if an empty list had been provided. But then again, everything is validated at the spec level.

I could change it to None and then mark the classad as undefined. But then it will be a job requiring GPU but asking for an undefined CUDA capability. I am not sure whether that could trigger other errors and unwanted behavior or not. Please let me know if you have any suggestions.

Copy link
Contributor

@todor-ivanov todor-ivanov Aug 17, 2023

Choose a reason for hiding this comment

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

Alan, I do not want to say it... but usually there are many ifs in the justification process of an unhealthy code behavior. Overlapping of default and failure return values is not a good practice in general. Let alone type modification. I know we are supposed to be protected on another level, and we should not enter this bit of code under bad conditions, but lets imagine something changes in the future and we change our policy regarding GPU job execution as well.... Then we may enter this code under unexpected conditions, or because of a bug in the request validation code.... anything may happen.

So I'd bet for returning a None value in case of a failure and let the upstream code deal with it. But that's up to you. If you think we are 100% safe and we are about to call this function under the best condition only at any possible moment in the future, we can leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really have a strong preference here. I can make it return None by default, have a check in SingleCondorPlugin for None and map that to undefined classad. Sounds good?

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, yes
thank you @amaltaro


For further details:
https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART____VERSION.html
"""
defaultRes = 0
# get an ordered list of the versions and use the very first element
capabilities = orderVersionList(capabilities)
if not capabilities:
Copy link
Contributor

Choose a reason for hiding this comment

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

If capabilities is not a list or None (which is the default for the method), but rather could be a dictionary or string... , because the function orderVersionList returns its argument as it is if it is not a list, then this protection here here won't stop the execution further, and the next line will break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the GPU parameters are validated at workflow creation (StdBase), so there shouldn't be a type different than list or None at this level

return defaultRes

smallestVersion = capabilities[0]
smallestVersion = smallestVersion.split(".")
# deal with versions like: "1", "1.2" and "1.2.3"
for _i in range(0, 3 - len(smallestVersion)):
smallestVersion.append(0)

intVersion = int(smallestVersion[0]) * 1000 + int(smallestVersion[1]) * 10 + int(smallestVersion[2])
return intVersion
7 changes: 5 additions & 2 deletions src/python/WMCore/BossAir/Plugins/SimpleCondorPlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -566,8 +566,11 @@ def getJobParameters(self, jobList):
if job.get('gpuRequirements', None):
ad['My.GPUMemoryMB'] = str(job['gpuRequirements']['GPUMemoryMB'])
cudaCapabilities = ','.join(sorted(job['gpuRequirements']['CUDACapabilities']))
ad['My.CUDACapability'] = classad.quote(str(cudaCapabilities))
ad['My.CUDARuntime'] = classad.quote(job['gpuRequirements']['CUDARuntime'])
minimalCapability = self.cudaCapabilityToSingleVersion(job['gpuRequirements']['CUDACapabilities'])
ad['My.CUDACapability'] = classad.quote(str(minimalCapability))
ad['My.OriginalCUDACapability'] = classad.quote(str(cudaCapabilities))
cudaRuntime = ','.join(sorted(job['gpuRequirements']['CUDARuntime']))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to sot the cudaRuntime variable as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am actually not sure whether this is needed or not. I was going to say to keep condor classad values consistent among different jobs of the same workflow/task, but it could be that this is already consistent/ordered in the original list. I'd keep it around in case the original list can come out with scrambled versions.

ad['My.CUDARuntime'] = classad.quote(str(cudaRuntime))
else:
ad['My.GPUMemoryMB'] = undefined
ad['My.CUDACapability'] = undefined
Expand Down
25 changes: 20 additions & 5 deletions src/python/WMCore/WMSpec/WMTask.py
Original file line number Diff line number Diff line change
Expand Up @@ -1525,16 +1525,31 @@ def getRequiresGPU(self):
def getGPURequirements(self):
"""
Return the GPU requirements for this task.
If it's a multi-step task, the first step with a meaningful
dictionary value will be returned
For multi-step tasks, the following logic is applied:
* GPUMemoryMB: return the max of them
* CUDARuntime: returns a flat list of unique runtime versions
* CUDACapabilities: returns a flat list of unique capabilities
:return: a dictionary with the GPU requirements for this task
"""
gpuRequirements = {}
gpuRequirements = []
for stepName in sorted(self.listAllStepNames()):
stepHelper = self.getStep(stepName)
if stepHelper.stepType() == "CMSSW" and stepHelper.getGPURequirements():
return stepHelper.getGPURequirements()
return gpuRequirements
gpuRequirements.append(stepHelper.getGPURequirements())
if not gpuRequirements:
return {}

# in this case, it requires GPUs and it can be multi-steps GPU
bestGPUParams = {"GPUMemoryMB": 0, "CUDARuntime": [], "CUDACapabilities": []}
for params in gpuRequirements:
if params["GPUMemoryMB"] > bestGPUParams["GPUMemoryMB"]:
bestGPUParams["GPUMemoryMB"] = params["GPUMemoryMB"]
bestGPUParams["CUDARuntime"].append(params["CUDARuntime"])
bestGPUParams["CUDACapabilities"].extend(params["CUDACapabilities"])
# make the flat list elements unique
bestGPUParams["CUDARuntime"] = list(set(bestGPUParams["CUDARuntime"]))
bestGPUParams["CUDACapabilities"] = list(set(bestGPUParams["CUDACapabilities"]))
return bestGPUParams

def _getStepValue(self, keyDict, defaultValue):
"""
Expand Down
12 changes: 11 additions & 1 deletion test/python/Utils_t/Utilities_t.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from Utils.Utilities import makeList, makeNonEmptyList, strToBool, \
safeStr, rootUrlJoin, zipEncodeStr, lowerCmsHeaders, getSize, \
encodeUnicodeToBytes, diskUse, numberCouchProcess
encodeUnicodeToBytes, diskUse, numberCouchProcess, orderVersionList


class UtilitiesTests(unittest.TestCase):
Expand Down Expand Up @@ -180,6 +180,16 @@ def testNumberCouchProcess(self):
# there should be at least one process, but who knows...
self.assertTrue(data >= 0)

def testOrderVersionList(self):
"""
Test the `orderVersionList` function.
"""
oldL = ["2.3.1", "1.2.3", "3.2.1", "1.3.2", "1.2"]
newL = ["1.2", "1.2.3", "1.3.2", "2.3.1", "3.2.1"]
with self.assertRaises(AssertionError):
self.assertListEqual(oldL, newL)
self.assertListEqual(orderVersionList(oldL), newL)


if __name__ == '__main__':
unittest.main()
21 changes: 21 additions & 0 deletions test/python/WMCore_t/BossAir_t/BasePlugin_t.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,26 @@ def testScramArchtoRequiredArch(self):

return

def testCudaCapabilityToSingleVersion(self):
"""
Test conversion of a list of version strings to a single integer version
"""
bp = BasePlugin(config=None)

# bad input
self.assertEqual(bp.cudaCapabilityToSingleVersion([]), 0)
self.assertEqual(bp.cudaCapabilityToSingleVersion({}), 0)
self.assertEqual(bp.cudaCapabilityToSingleVersion(None), 0)
# good and expected input
unorderedL = ["2.3.1", "1.2.3", "3.2.1", "1.3.2", "1.2"]
self.assertEqual(bp.cudaCapabilityToSingleVersion(unorderedL), 1020)
orderedL = ["1.2", "1.2.3", "1.3.2", "2.3.1", "3.2.1"]
self.assertEqual(bp.cudaCapabilityToSingleVersion(orderedL), 1020)
orderedL = ["1.2.3", "1.3.2", "2.3.1", "3.2.1"]
self.assertEqual(bp.cudaCapabilityToSingleVersion(orderedL), 1023)
orderedL = ["2.3.1", "3.2.1"]
self.assertEqual(bp.cudaCapabilityToSingleVersion(orderedL), 2031)


if __name__ == '__main__':
unittest.main()
28 changes: 14 additions & 14 deletions test/python/WMCore_t/WMSpec_t/StdSpecs_t/StepChain_t.py
Original file line number Diff line number Diff line change
Expand Up @@ -2489,9 +2489,10 @@ def testGPUStepChainsTasks(self):
testArguments[s]['ConfigCacheID'] = configDocs[s]
testArguments['Step2']['KeepOutput'] = False

gpuParams = {"GPUMemoryMB": 1234, "CUDARuntime": "11.2.3", "CUDACapabilities": ["7.5", "8.0"]}
testArguments['Step1'].update({"RequiresGPU": "optional", "GPUParams": json.dumps(gpuParams)})
testArguments['Step2'].update({"RequiresGPU": "required", "GPUParams": json.dumps(gpuParams)})
gpuParams1 = {"GPUMemoryMB": 1234, "CUDARuntime": "11.2.3", "CUDACapabilities": ["7.5", "8.0"]}
testArguments['Step1'].update({"RequiresGPU": "optional", "GPUParams": json.dumps(gpuParams1)})
gpuParams2 = {"GPUMemoryMB": 2345, "CUDARuntime": "9.6", "CUDACapabilities": ["7.4"]}
testArguments['Step2'].update({"RequiresGPU": "required", "GPUParams": json.dumps(gpuParams2)})
factory = StepChainWorkloadFactory()
testWorkload = factory.factoryWorkloadConstruction("TestWorkload", testArguments)

Expand All @@ -2503,8 +2504,8 @@ def testGPUStepChainsTasks(self):

# validate GPU parameters
self.assertEqual(testArguments['GPUParams'], json.dumps(None))
self.assertEqual(testArguments["Step1"]['GPUParams'], json.dumps(gpuParams))
self.assertEqual(testArguments["Step2"]['GPUParams'], json.dumps(gpuParams))
self.assertEqual(testArguments["Step1"]['GPUParams'], json.dumps(gpuParams1))
self.assertEqual(testArguments["Step2"]['GPUParams'], json.dumps(gpuParams2))
self.assertTrue("GPUParams" not in testArguments["Step3"])

for taskName in testWorkload.listAllTaskNames():
Expand All @@ -2520,10 +2521,10 @@ def testGPUStepChainsTasks(self):
elif stepHelper.stepType() == "CMSSW" and taskName == "GENSIM":
if stepHelper.name() == "cmsRun1":
self.assertEqual(stepHelper.data.application.gpu.gpuRequired, testArguments["Step1"]['RequiresGPU'])
self.assertItemsEqual(stepHelper.data.application.gpu.gpuRequirements, gpuParams)
self.assertItemsEqual(stepHelper.data.application.gpu.gpuRequirements, gpuParams1)
elif stepHelper.name() == "cmsRun2":
self.assertEqual(stepHelper.data.application.gpu.gpuRequired, testArguments["Step2"]['RequiresGPU'])
self.assertItemsEqual(stepHelper.data.application.gpu.gpuRequirements, gpuParams)
self.assertItemsEqual(stepHelper.data.application.gpu.gpuRequirements, gpuParams2)
elif stepHelper.name() == "cmsRun3":
self.assertEqual(stepHelper.data.application.gpu.gpuRequired, "forbidden")
self.assertIsNone(stepHelper.data.application.gpu.gpuRequirements)
Expand All @@ -2535,18 +2536,17 @@ def testGPUStepChainsTasks(self):
prodTask = testWorkload.getTask('GENSIM')
gpuRequired, gpuRequirements = prodTask.getStepHelper('cmsRun1').getGPUSettings()
self.assertEqual(gpuRequired, testArguments["Step1"]['RequiresGPU'])
self.assertItemsEqual(gpuRequirements, gpuParams)
self.assertItemsEqual(gpuRequirements, gpuParams1)

gpuRequired, gpuRequirements = prodTask.getStepHelper('cmsRun2').getGPUSettings()
self.assertEqual(gpuRequired, testArguments["Step2"]['RequiresGPU'])
self.assertItemsEqual(gpuRequirements, gpuParams)
self.assertItemsEqual(gpuRequirements, gpuParams2)

gpuRequired, gpuRequirements = prodTask.getStepHelper('cmsRun3').getGPUSettings()
self.assertEqual(gpuRequired, testArguments["Step3"].get('RequiresGPU', "forbidden"))
self.assertIsNone(gpuRequirements)



# test assignment with wrong Trust flags
assignDict = {"SiteWhitelist": ["T2_US_Nebraska"], "Team": "The-A-Team",
"RequestStatus": "assigned"}
Expand All @@ -2560,18 +2560,18 @@ def testGPUStepChainsTasks(self):

# validate GPU parameters
self.assertEqual(testArguments['GPUParams'], json.dumps(None))
self.assertEqual(testArguments["Step1"]['GPUParams'], json.dumps(gpuParams))
self.assertEqual(testArguments["Step2"]['GPUParams'], json.dumps(gpuParams))
self.assertEqual(testArguments["Step1"]['GPUParams'], json.dumps(gpuParams1))
self.assertEqual(testArguments["Step2"]['GPUParams'], json.dumps(gpuParams2))
self.assertTrue("GPUParams" not in testArguments["Step3"])

prodTask = testWorkload.getTask('GENSIM')
gpuRequired, gpuRequirements = prodTask.getStepHelper('cmsRun1').getGPUSettings()
self.assertEqual(gpuRequired, testArguments["Step1"]['RequiresGPU'])
self.assertItemsEqual(gpuRequirements, gpuParams)
self.assertItemsEqual(gpuRequirements, gpuParams1)

gpuRequired, gpuRequirements = prodTask.getStepHelper('cmsRun2').getGPUSettings()
self.assertEqual(gpuRequired, testArguments["Step2"]['RequiresGPU'])
self.assertItemsEqual(gpuRequirements, gpuParams)
self.assertItemsEqual(gpuRequirements, gpuParams2)

gpuRequired, gpuRequirements = prodTask.getStepHelper('cmsRun3').getGPUSettings()
self.assertEqual(gpuRequired, testArguments["Step3"].get('RequiresGPU', "forbidden"))
Expand Down
2 changes: 2 additions & 0 deletions test/python/WMCore_t/WMSpec_t/WMTask_t.py
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,8 @@ def testGPUTaskSettings(self):
### Now set a single value for both tasks
gpuParams = {"GPUMemoryMB": 1234, "CUDARuntime": "11.2.3", "CUDACapabilities": ["7.5", "8.0"]}
task1.setTaskGPUSettings("required", json.dumps(gpuParams))
# CUDARuntime returns as a list
gpuParams["CUDARuntime"] = [gpuParams["CUDARuntime"]]
for taskObj in task1.taskIterator():
# task level check
self.assertEqual(taskObj.getRequiresGPU(), "required")
Expand Down