-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding new filter rule(Ubuntu 20.04 does not support CUDA <10.2) #50
Adding new filter rule(Ubuntu 20.04 does not support CUDA <10.2) #50
Conversation
I did a mistake in the issue description. CUDA 10.2 and older is not available in Ubuntu 20.04. You can check for |
src/bashi/filter_backend.py
Outdated
@@ -143,6 +143,12 @@ def backend_filter( | |||
return False | |||
break | |||
|
|||
# Rule: b18 | |||
# related to rule | |||
if row[ALPAKA_ACC_GPU_CUDA_ENABLE].version < pkv.parse("10.2"): |
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.
if row[ALPAKA_ACC_GPU_CUDA_ENABLE].version < pkv.parse("10.2"): | |
if row[ALPAKA_ACC_GPU_CUDA_ENABLE].version < pkv.parse("11.0"): |
return False | ||
for compiler in (HOST_COMPILER, DEVICE_COMPILER): | ||
if compiler in row and row[compiler].name == CLANG_CUDA: | ||
if row[compiler].version < CLANG_CUDA_MAX_CUDA_VERSION[3].clang_cuda: |
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.
Good point, that you check also for Clang-CUDA
:-) Thanks correct and didn't thought about it.
Unfortunately, there is no guaranty that CLANG_CUDA_MAX_CUDA_VERSION
keeps his ordering. Therefore you need to replace the direct access with a search function. Otherwise your code could silently break, if somebody modifies the CLANG_CUDA_MAX_CUDA_VERSION
list. You need to implement the following function:
def get_max_supported_clang_cuda_ver(cuda_ver : str) -> str:
""" returns the latest supported clang-cuda version for a given cuda version"
# implement search algorithm
# don't expect specific ordering of the list CLANG_CUDA_MAX_CUDA_VERSION
# implement also tests
Implement the function in the version.py
.
By the way, an entry of CLANG_CUDA_MAX_CUDA_VERSION
stores only string. Therefore you need to parse the version. At the moment, you compare version objects with strings, which is always wrong.
The versions in ClangCudaSDKSupport
are already parsed and stored as packaging.version.Version
.
Lines 51 to 64 in cc789bd
class ClangCudaSDKSupport(VersionSupportBase): | |
"""Contains a nvcc version and host compiler version. Does automatically parse the input strings | |
to package.version.Version. | |
Provides comparision operators for sorting. | |
""" | |
def __init__(self, clang_cuda_version: str, cuda_version: str): | |
VersionSupportBase.__init__(self, clang_cuda_version, cuda_version) | |
self.clang_cuda = self.version1 | |
self.cuda = self.version2 | |
def __str__(self) -> str: | |
return f"Clang-CUDA {str(self.clang_cuda)} + CUDA SDK {self.cuda}" |
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.
I suggest that the interface takes a string and the first step is parsing it to a version object. Makes the function call easier:
# expected string as argument (preferred version)
get_max_supported_clang_cuda_ver("11.2")
# expected version object as argument
get_max_supported_clang_cuda_ver(pkv.parse("11.2")
# implementation for the preferred version
def get_max_supported_clang_cuda_ver(cuda_ver : str) -> str:
""" returns the latest supported clang-cuda version for a given cuda version"
parsed_cuda_ver: pkv.Version = pkv.parse(cuda_ver)
# implement search algorithm
# don't expect specific ordering of the list CLANG_CUDA_MAX_CUDA_VERSION
# implement also tests
src/bashi/results.py
Outdated
@@ -792,3 +793,31 @@ def _remove_unsupported_cmake_versions_for_clangcuda( | |||
value_name2=CMAKE, | |||
value_version2=">3.18", | |||
) | |||
|
|||
|
|||
def _remove_unsupported_cuda_versions_for_ubuntu( |
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.
This function needs also to remove unsupported device compiler nvcc + ubuntu 18.04
combinations and clang-cuda + ubuntu 18.04
. I saw that the example.py misses the following combinations:
ParameterValuePair(first=ParameterValueSingle(parameter='device_compiler', parameterValue=ParameterValue(name='nvcc', version=<Version('10.0')>)), second=ParameterValueSingle(parameter='ubuntu', parameterValue=ParameterValue(name='ubuntu', version=<Version('20.4')>))) is missing in combination list
ParameterValuePair(first=ParameterValueSingle(parameter='device_compiler', parameterValue=ParameterValue(name='nvcc', version=<Version('10.1')>)), second=ParameterValueSingle(parameter='ubuntu', parameterValue=ParameterValue(name='ubuntu', version=<Version('20.4')>))) is missing in combination list
But we cannot install CUDA 10.0 and 10.1 on Ubuntu 20.04
I strongly recommend to fix the function For example the |
3327500
to
164477f
Compare
src/bashi/results.py
Outdated
parameter_value_pairs_ranges: List[ParameterValuePair], | ||
removed_parameter_value_pairs: List[ParameterValuePair], | ||
): | ||
removed_parameter_value_pairs( | ||
parameter_value_pairs_ranges, | ||
removed_parameter_value_pairs, |
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.
Instead calling the new function, you simply renamed the variable.
parameter_value_pairs_ranges: List[ParameterValuePair], | |
removed_parameter_value_pairs: List[ParameterValuePair], | |
): | |
removed_parameter_value_pairs( | |
parameter_value_pairs_ranges, | |
removed_parameter_value_pairs, | |
parameter_value_pairs: List[ParameterValuePair], | |
removed_parameter_value_pairs: List[ParameterValuePair], | |
): | |
remove_parameter_value_pairs_ranges( | |
parameter_value_pairs, | |
removed_parameter_value_pairs, |
73cd175
to
1cefd4e
Compare
src/bashi/results.py
Outdated
value_name1=UBUNTU, | ||
value_min_version1=20.04, | ||
value_min_version1_inclusive=True, | ||
value_max_version1=9999, |
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.
You can remove this argument. If you use the default argument for value_max_version1
, it will delete all version bigger than 20.04, including 20,04.
src/bashi/results.py
Outdated
value_name1=UBUNTU, | ||
value_min_version1=20.04, | ||
value_min_version1_inclusive=True, | ||
value_max_version1=9999, |
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.
Use the default argument, means the argument value_max_version1=9999,
.
src/bashi/results.py
Outdated
value_name1=UBUNTU, | ||
value_min_version1=20.04, | ||
value_min_version1_inclusive=True, | ||
value_max_version1=9999, |
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.
Use the default argument, means the argument value_max_version1=9999,
.
), | ||
] | ||
) | ||
expected_results = parse_expected_val_pairs( |
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.
Was some work to review this, but the expected results looks correct and the test in general. Remove the the value_max_version1=9999,
and the validation site is ready. Therefore only the filter rules needs to be fixed.
src/bashi/results.py
Outdated
value_min_version2=OFF, | ||
value_min_version2_inclusive=False, | ||
value_max_version2=11, | ||
value_max_version2_inclusive=True, |
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.
value_max_version2_inclusive=True, | |
value_max_version2_inclusive=False, |
Needs to be exclusive, because otherwise you remove the pair Ubuntu 20.04 + CUDA 11.0.
Please also the pair to the tests. This corner case is not covered by your test casese. Only by the long running test.
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), |
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.
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), | |
DEVICE_COMPILER: ppv((NVCC, nvcc_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), |
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.
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), | |
DEVICE_COMPILER: ppv((NVCC, nvcc_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), |
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.
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), | |
DEVICE_COMPILER: ppv((NVCC, nvcc_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
DEVICE_COMPILER: ppv((DEVICE_COMPILER, clang_cuda_version)), |
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.
DEVICE_COMPILER: ppv((DEVICE_COMPILER, clang_cuda_version)), | |
DEVICE_COMPILER: ppv((CLANG_CUDA, clang_cuda_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), |
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.
DEVICE_COMPILER: ppv((DEVICE_COMPILER, nvcc_version)), | |
DEVICE_COMPILER: ppv((NVCC, nvcc_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
DEVICE_COMPILER: ppv((DEVICE_COMPILER, clang_cuda_version)), |
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.
DEVICE_COMPILER: ppv((DEVICE_COMPILER, clang_cuda_version)), | |
DEVICE_COMPILER: ppv((CLANG_CUDA, clang_cuda_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
HOST_COMPILER: ppv((HOST_COMPILER, clang_cuda_version)), |
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.
HOST_COMPILER: ppv((HOST_COMPILER, clang_cuda_version)), | |
HOST_COMPILER: ppv((CLANG_CUDA, clang_cuda_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
DEVICE_COMPILER: ppv((DEVICE_COMPILER, clang_cuda_version)), |
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.
DEVICE_COMPILER: ppv((DEVICE_COMPILER, clang_cuda_version)), | |
DEVICE_COMPILER: ppv((CLANG_CUDA, clang_cuda_version)), |
software_dependency_filter_typechecked( | ||
OD( | ||
{ | ||
HOST_COMPILER: ppv((HOST_COMPILER, clang_cuda_version)), |
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.
HOST_COMPILER: ppv((HOST_COMPILER, clang_cuda_version)), | |
HOST_COMPILER: ppv((CLANG_CUDA, clang_cuda_version)), |
@YuriiPerets You did the same mistake with the host or device compiler parameter-value-pair many times. Please check also by yourself, if I missed to marked it in your code somewhere and fix it by yourself. |
231e879
to
61ee9f1
Compare
): | ||
reason( | ||
output, | ||
f"{__pretty_name_compiler(compiler_type)} clang cuda {row[compiler_type].version} " |
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.
f"{__pretty_name_compiler(compiler_type)} clang cuda {row[compiler_type].version} " | |
f"{__pretty_name_compiler(compiler_type)} clang-cuda {row[compiler_type].version} " |
Just a little bit sugar ;-)
@YuriiPerets Nice work. Please fix this one reason message, than squash and rebase please. Hopefully if you did everything and force push, the CI will be triggered again. I executed the test locally and it looks good. |
61ee9f1
to
285f153
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 94.94% 95.14% +0.20%
==========================================
Files 10 10
Lines 672 700 +28
Branches 202 213 +11
==========================================
+ Hits 638 666 +28
Misses 15 15
Partials 19 19 ☔ View full report in Codecov by Sentry. |
fix: #32