Skip to content

Task/all of us/basic blt testing - #18

Open
white238 wants to merge 20 commits into
masterfrom
task/all_of_us/basic_blt_testing
Open

Task/all of us/basic blt testing#18
white238 wants to merge 20 commits into
masterfrom
task/all_of_us/basic_blt_testing

Conversation

@white238

@white238 white238 commented Dec 7, 2021

Copy link
Copy Markdown
Member

No description provided.

@kennyweiss kennyweiss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this waiting on anything before merging? Or can we merge sooner and update/patch as needed?

view: false
concretization: separately
specs:
- blttest

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing I've been wondering is how we can support multiple specs within an environment, e.g. when we add more compilers.

Would we do something like:

spack:
  concretization: separately
  specs:
  - blttest%clang@10.0.0
  - blttest%gcc@8.3.1

Or would we have one environment per compiler/spec ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One thing I've been wondering is how we can support multiple specs within an environment, e.g. when we add more compilers.

Would we do something like:

spack:
  concretization: separately
  specs:
  - blttest%clang@10.0.0
  - blttest%gcc@8.3.1

Or would we have one environment per compiler/spec ?

You could use Spack stacks (https://spack-tutorial.readthedocs.io/en/latest/tutorial_stacks.html), where you'd define specs and define compilers and Spack would build each spec with each compiler.

@tldahlgren tldahlgren Dec 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can see a relatively simple example at https://github.com/LLNL/radiuss-spack-testing/blob/dahlgren1/add-care-env/spack-environments/care/spack.yaml. Specifically the definitions starting at line 29/30 and matrix at lines 46-48.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there is value in testing individual spack envs - since that is most likely how users will build when developing. But again, that is tied to my deployment model where I use a config to dial everything in to deploy with a specific set of features on a specific platform, and then I use multiple configs when I need more than one deployments (for example, cuda only, vs openmp only). Maybe thats not an important goal for the blt testing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can also specify multiple matrixs. There's a somewhat simple example at https://github.com/mpbelhorn/olcf-spack-environments/blob/develop/hosts/lyra/envs/base/spack.yaml.

@tldahlgren tldahlgren left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FWIW. Just a few suggestions.

if ((self.compiler.fc is not None)
and ("gfortran" in self.compiler.fc)
and ("clang" in self.compiler.cxx)):
libdir = pjoin(os.path.dirname(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Spack folks prefer using join_path provided by Spack instead of os.path.join.


if (self.compiler.fc is not None) and ("xlf" in self.compiler.fc):
# Grab lib directory for the current fortran compiler
libdir = pjoin(os.path.dirname(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same join_path suggestion as above.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's another at line 225.

Comment on lines +255 to +258
if self.run_tests is False:
options.append('-DENABLE_TESTS=OFF')
else:
options.append('-DENABLE_TESTS=ON')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you not simply:

Suggested change
if self.run_tests is False:
options.append('-DENABLE_TESTS=OFF')
else:
options.append('-DENABLE_TESTS=ON')
options.append(self.define('ENABLE_TESTS', self.run_tests))

Comment on lines +265 to +267
@run_after('build')
@on_package_attributes(run_tests=True)
def build_test(self):

@tldahlgren tldahlgren Dec 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you not just override the check method (and skip the directives) since Spack automatically calls check after the build phase when inheriting from CMakePackage (unless you override build_time_test_callbacks)?

Comment on lines +218 to +221
_roots = ["/usr/tce/packages/gcc/gcc-4.9.3",
"/usr/tce/packages/gcc/gcc-4.9.3/gnu"]
_subdirs = ["lib64",
"lib64/gcc/powerpc64le-unknown-linux-gnu/4.9.3"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not use join_path here instead of explicitly entering / in the paths?

Comment on lines +150 to +154
if not spec.satisfies('cuda_arch=none'):
cuda_arch = spec.variants['cuda_arch'].value[0]
entries.append(cmake_cache_string(
"CMAKE_CUDA_ARCHITECTURES",
cuda_arch))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I realize using spec.satisfies() for this check is common for RADIUSS packages, but I believe you could do something like the following:

Suggested change
if not spec.satisfies('cuda_arch=none'):
cuda_arch = spec.variants['cuda_arch'].value[0]
entries.append(cmake_cache_string(
"CMAKE_CUDA_ARCHITECTURES",
cuda_arch))
cuda_arch = spec.variants['cuda_arch'].value
if cuda_arch != 'none':
entries.append(cmake_cache_string(
"CMAKE_CUDA_ARCHITECTURES",
cuda_arch[0]))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants