-
Notifications
You must be signed in to change notification settings - Fork 5
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
RPP Test suite : Modifications to support python 2 #288
RPP Test suite : Modifications to support python 2 #288
Conversation
HazarathKumarM
commented
Jul 11, 2024
- Adds changes to support tests for all the versions of python
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.
@HazarathKumarM Please address comments
result = subprocess.run([buildFolderPath + "/build/Tensor_misc_hip", str(case), str(testType), str(toggle), str(numDims), str(batchSize), str(numRuns), str(additionalArg), outFilePath, scriptPath], stdout=subprocess.PIPE) # nosec | ||
print(result.stdout.decode()) | ||
print("./Tensor_misc_hip {} {} {} {} {} {} {}".format(case, testType, toggle, numDims, batchSize, numRuns, additionalArg)) | ||
result = subprocess.Popen([buildFolderPath + "/build/Tensor_misc_hip", str(case), str(testType), str(toggle), str(numDims), str(batchSize), str(numRuns), str(additionalArg), outFilePath, scriptPath], stdout=subprocess.PIPE) # nosec |
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.
Why the change from subprocess.run to subprocess.Popen here?
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.
.run methos is only introduced in python3.5 , so we need to change it to Popen to support running in python2
subprocess.run(["cmake", scriptPath], cwd=".") # nosec | ||
subprocess.run(["make", "-j16"], cwd=".") # nosec | ||
subprocess.call(["cmake", scriptPath], cwd=".") # nosec | ||
subprocess.call(["make", "-j16"], cwd=".") # nosec |
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.
What's the difference in changing from run to call? vs the Popen above?
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.
.call returns either 0 or 1 as output, in the above case we need to print the log in terminal and as well as to the log file , we need stdout to be returned from the subprocess, Popen return that log
utilities/test_suite/HIP/runTests.py
Outdated
os.makedirs(f"{dstPath}/Tensor_PLN1") | ||
os.makedirs(f"{dstPath}/Tensor_PLN3") | ||
os.makedirs("{}/Tensor_PKD3".format(dstPath)) | ||
os.makedirs("{}/Tensor_PLN1".format(dstPath)) |
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.
Doesn't string concatenation work for all Python versions? Like using (dstPath + "/Tensor_PKD3")?
Or is the .format() useful somewhere else?
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.
changed to concatenation
@r-abishek currently there are no deprecation warnings for .call() and .Popen() methods |
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.
@HazarathKumarM
Test confirmation on python 2.7, 3.5, 3.10