Skip to content

Conversation

@rsarm
Copy link
Contributor

@rsarm rsarm commented Mar 17, 2021

Closes #1466

@codecov-io
Copy link

Codecov Report

Merging #1871 (13bc90a) into master (a3d0b0c) will increase coverage by 0.04%.
The diff coverage is 36.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1871      +/-   ##
==========================================
+ Coverage   87.61%   87.65%   +0.04%     
==========================================
  Files          49       49              
  Lines        8136     8504     +368     
==========================================
+ Hits         7128     7454     +326     
- Misses       1008     1050      +42     
Impacted Files Coverage Δ
reframe/core/buildsystems.py 88.49% <36.17%> (-7.74%) ⬇️
reframe/core/variables.py 94.83% <0.00%> (-1.64%) ⬇️
reframe/core/decorators.py 97.05% <0.00%> (-0.03%) ⬇️
reframe/core/meta.py 100.00% <0.00%> (ø)
reframe/frontend/loader.py 92.50% <0.00%> (ø)
reframe/utility/__init__.py 92.12% <0.00%> (ø)
reframe/frontend/executors/policies.py 99.69% <0.00%> (+0.02%) ⬆️
reframe/core/parameters.py 98.50% <0.00%> (+0.04%) ⬆️
reframe/frontend/executors/__init__.py 98.29% <0.00%> (+0.08%) ⬆️
reframe/core/pipeline.py 92.03% <0.00%> (+0.12%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3d0b0c...13bc90a. Read the comment docs.

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

@rsarm, it works great for me, except when I am using with Lmod with hierarchical naming scheme... I think we should have a dedicated solution for that.

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

@rsarm, I forgot to add the issue that I have experienced with this build system. Please take a look at the comment about source_cache and misc_cache.

@pep8speaks
Copy link

pep8speaks commented Apr 8, 2021

Hello @rsarm, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2021-05-25 15:14:31 UTC

@rsarm
Copy link
Contributor Author

rsarm commented Apr 8, 2021

Example checks:

import reframe as rfm
import reframe.utility.sanity as sn

class SpackMixin(rfm.RegressionTest):
    @rfm.run_before('run')
    def prepare_run(self):
        self.prerun_cmds = [
            f'spack env activate -d {self.build_system.environment}'
        ]
        self.prerun_cmds.append([
            f'spack load {mod}'
            for mod in self.build_system.generated_modules
        ])


@rfm.simple_test
class SpackEnvCheck(SpackMixin):
    def __init__(self):
        self.descr = 'Spack buildsystem example.'
        self.valid_systems = ['*']  # ['daint:gpu', 'dom:gpu']
        self.valid_prog_environs = ['*']  # ['builtin']
        self.sourcesdir += '/spack'
        self.build_system = 'Spack'
        self.executable = 'iconv'
        self.executable_opts = ['--version']
        self.build_system.environment = 'spack_env'
        self.sanity_patterns = sn.assert_found(
            r'iconv \(GNU libiconv 1\.16\)', self.stdout
        )

with the environment

spack:
  specs:
    - zlib@1.2.3
    - libiconv@1.16
  concretization: together
  config:
    install_tree: spack/opt/spack
    module_roots:
      tcl: spack/share/spack/modules
      lmod: spack/share/spack/lmod

that goes in src/spack/spack.yaml.

This test runs fine.

The following test using 'scopes' gives the error as if spack was not properly setup (at least with the local scheduler):

import reframe as rfm
import reframe.utility.sanity as sn

class SpackMixin(rfm.RegressionTest):
    @rfm.run_before('run')
    def prepare_run(self):
        self.prerun_cmds = [
            f'spack -C {self.build_system.scope_dir} load {mod}'
            for mod in self.build_system.generated_modules
        ]
        # self.executable = 'spack'
        # self.executable_opts = [f'-C {self.build_system.scope_dir} find']


@rfm.simple_test
class UlimitCheck(SpackMixin):
    def __init__(self):
        self.descr = 'Checking the output of ulimit -s in node.'
        self.valid_systems = ['*']  # ['daint:gpu', 'dom:gpu']
        self.valid_prog_environs = ['*']  # ['builtin']
        self.sourcesdir += '/ulimit'
        self.build_system = 'Spack'
        self.build_system.packages = ['libiconv@1.16',
                                      'zlib@1.2.3',
                                      'openmpi@4.0.5']
        self.build_system.upstreams = ['/home/sarafael/spack/opt/spack']
        self.executable = 'iconv'
        self.executable_opts = ['--version']
        self.sanity_patterns = sn.assert_found(
            r'iconv \(GNU libiconv 1\.16\)', self.stdout
        )

It creates the script

#!/bin/bash
spack -C rfm_spack_scope load libiconv@1.16/af5tdk6ilv6mah2ntgb5odryvlosijnz
spack -C rfm_spack_scope load zlib@1.2.3/5e4xv5gq7liaii4iwktakwk3gqjchlll
spack -C rfm_spack_scope load openmpi@4.0.5/622d22gh5n7vhilmy6seea53cyo3z523
 iconv --version

which gives the error:

Error: `spack load` requires spack's shell support.

@rsarm rsarm changed the title [wip][feat] Use Spack as a build system [feat] Use Spack as a build system Apr 8, 2021
@rsarm rsarm marked this pull request as ready for review April 8, 2021 09:59
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Generally, looks good to me. We need to expand the documentation, as well as write a minimal unit test the check the emitted commands as we for the EB. Finally, did we resolve the issue about spack load?

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

What we are still missing is a way to automatically activate the environment and load the modules when the test runs. One way I see this happening is by expanding the BuildSystem basic interface with a function that would return commands to be emitted before running the test. And we could also add a boolean field to the Spack backend to enable or disable the automatic generation of these commands.

And, finally, we also miss the documentation.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

I tried the PR and I have a comment:

I am using the fish shell, so in my case I have to source Spack's setup-env.sh in each of the scripts (build and run). Although, sourcing it in prebuild_cmds it will work for the build phase, it won't do so if I add it to prerun_cmds for the run phase, because the spack env activate is issued before these commands. Could we automatically generate the sourcing of the setup-env.sh script? Could we get the prefix of the spack installation for example?

rsarm and others added 3 commits May 19, 2021 13:23
- Emit `spack load` instructions in run phase by default.
Vasileios Karakasis added 5 commits May 22, 2021 17:40
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

lgtm now. I have done some improvements and added a couple of improvements.

@vkarak vkarak requested a review from victorusu May 22, 2021 20:54
Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

lgtm

@vkarak
Copy link
Contributor

vkarak commented May 25, 2021

@jenkins-cscs retry daint

@vkarak vkarak changed the title [feat] Use Spack as a build system [feat] Add a new build system backend for building test code with Spack May 25, 2021
@vkarak vkarak merged commit 8bd06ea into reframe-hpc:master May 25, 2021
@rsarm rsarm deleted the feat/spack-buildsystem branch February 3, 2023 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use Spack as a build system

6 participants