Make clingo the default solver by alalazo · Pull Request #25502 · spack/spack

alalazo · 2021-08-19T08:12:52Z

Modifications:

Change defaults/config.yaml
Add a fix for bootstrapping patchelf from sources if compilers.yaml is empty
Make SPACK_TEST_SOLVER=clingo the default for unit-tests
Fix package failures in the e4s pipeline

Caveats:

CentOS 6 still uses the original concretizer as it can't connect to the buildcache due to issues with ssl (bootstrapping from sources requires a C++14 capable compiler)
I had to update the image tag for GitlabCI in e699f14. I did this since Radiuss is using that image and is already working with clingo, but it's not clear to me what are the differences between the two tags, so asked for @scottwittenburg review of that change. Thanks for the review @scottwittenburg
libtool v2.4.2 has been deprecated and other packages received some update

scottwittenburg · 2021-08-19T15:15:51Z

share/spack/gitlab/cloud_pipelines/.gitlab-ci.yml


 default:
-  image: { "name": "ghcr.io/scottwittenburg/ecpe4s-ubuntu18.04-runner-x86_64:2020-09-01", "entrypoint": [""] }
+  image: { "name": "ghcr.io/scottwittenburg/ecpe4s-ubuntu18.04-runner-x86_64:2021-05-15", "entrypoint": [""] }


Did you already check that the compilers this image has are the same as the 2020-09-01 tagged image? If not, pipeline jobs will end up doing a lot of duplicated bootstrapping. So in that case, we would either need to change the compiler we specify in the stacks that use the default image (at least share/spack/gitlab/cloud_pipelines/stacks/e4s/spack.yaml, but also maybe others), or else make a new image with the desired compilers already installed.

No, I didn't. For build-systems it might not matter but I think it matters for e4s. I'll stop the pipeline.

@tgamblin Would it be fine with you if in this PR I add logic to use the original concretizer for the e4s pipeline, until #25287 is ready to be merged?

Phew, that explains why it was canceled! 😅 I was worried there for a sec, when I didn't see any new push or request for spackbot to start a pipeline.

So, after a chat on slack with @eugeneswalker it seems to me that we'll be using the same compilers as before, so I'll restart the pipeline and see how many rebuilds it triggers.

alalazo · 2021-08-19T15:33:54Z

@spackbot run pipeline

spackbot-app · 2021-08-19T15:33:57Z

I've started that pipeline for you!

etc/spack/defaults/config.yaml

becker33 · 2021-08-19T20:34:37Z

lib/spack/spack/bootstrap.py

+    # We may need to compile code from sources, so ensure we have compilers
+    # for the current platform before switching parts. We use "spack compiler
+    # list" as it searches for compilers lazily i.e. only if we don't have
+    # them already in compilers.yaml
+    compiler_cmd = spack.main.SpackCommand('compiler')
+    compiler_cmd(
+        'list', output=os.devnull, error=os.devnull, fail_on_error=False
+    )


This doesn't actually ensure we have any compilers available for the current architecture -- if you have only compilers for a different OS, spack compiler list still won't find new ones.

We should explicitly call into the spack.compilers module and check whether we have any compilers for the current architecture.

Updated the logic in 96ac691. Spack looks into compilers.yaml first and calls:

spack compiler find

only if we don't have a compiler for the current host.

lib/spack/spack/bootstrap.py

lib/spack/spack/environment.py

alalazo · 2021-08-24T12:50:54Z

@becker33 @tgamblin Tests are passing, let me know if there are further modifications to be done here.

haampie · 2021-08-24T22:25:30Z

lib/spack/spack/bootstrap.py

+    arch = spack.architecture.default_arch()
+    arch = spack.spec.ArchSpec(str(arch))  # The call below expects an ArchSpec object
+    if not spack.compilers.compilers_for_arch(arch):
+        compiler_cmd = spack.main.SpackCommand('compiler')


are we using SpackCommand in our code directly now?

Callable object that invokes a spack command (for testing).

is in its docstring

Would rather not use it in the code, but in a pinch we can. It would be better to factor this so that it does not go through SpackCommand IMO.

We use SpackCommand in two places in bootstrap.py. The fact is our cli commands are not always thin wrappers of API calls but they may have a fair amount of logic in them. I agree this should be refactored, but I'd prefer to do it in a PR focused on refactoring the API - so that it wouldn't be out of place refactoring other related calls in an internal module and the PR would be focused on a single objective.

alalazo · 2021-08-28T07:12:51Z

@spackbot run pipeline

spackbot-app · 2021-08-28T07:12:53Z

I've started that pipeline for you!

scottwittenburg · 2021-09-10T17:37:41Z

fyi @eugeneswalker

tldahlgren

LGTM.

becker33

Are the libtool/numactl changes really a part of this PR? Is that something that currently fails in clingo?

Mostly LGTM.

becker33 · 2021-09-13T20:37:37Z

.github/workflows/unit_tests.yaml

          git fetch origin ${{ github.ref }}:test-branch
          git checkout test-branch
-          share/spack/qa/run-unit-tests
+          bin/spack unit-test -x -m "not maybeslow" -k "package_sanity"


Is there a reason we're not running the shell tests anymore?

The libtool change was a quick workaround a month ago to not select libtool 2.4.2 in DAGs. The real issue has been then fixed in #25585 but since the package update also made sense in itself I kept it. I can extract that into another PR if it makes more sense.

Is there a reason we're not running the shell tests anymore?

It's because of point 1. in the description (note that we are not running shell tests anymore only in the CentOS 6 tests):

CentOS 6 still uses the original concretizer as it can't connect to the buildcache due to issues with ssl (bootstrapping from sources requires a C++14 capable compiler)

I encountered issues with CentOS 6 in CI due apparently to using an old Python interpreter. Since it is my understanding (or maybe it's just a strong hope 😬 ) that the second after we release v0.17.0 we also drop Python 2.6 I worked around that issue by using the original concretizer and avoiding the:

$ spack spec mpileaks

we have in the fast shell tests. Honestly I didn't investigate further the SSL error that was reported with Python 2.6, but let me know if I should dig deeper into it.

- [x] Change defaults/config.yaml - [x] Add a fix for bootstrapping patchelf from sources if compilers.yaml is empty - [x] Make SPACK_TEST_SOLVER=clingo the default for unit-tests

We cannot download information from the binary caches: ==> [2021-08-19-09:26:29.002062] Unable to read index hash https://mirror.spack.io/bootstrap/github-actions/v0.1/build_cache/index.json.hash Download failed: <urlopen error [Errno 1] _ssl.c:492: error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol> 1

Remove preferences that may direct the solver towards libtool@develop Add a conflict to numactl

alalazo · 2021-09-14T06:11:24Z

@spackbot run pipeline

spackbot-app · 2021-09-14T06:11:26Z

I've started that pipeline for you!

alalazo · 2021-09-14T11:39:35Z

@spackbot run pipeline

spackbot-app · 2021-09-14T11:39:37Z

I've started that pipeline for you!

alalazo · 2021-09-14T13:52:59Z

@spackbot run pipeline

spackbot-app · 2021-09-14T13:53:01Z

I've started that pipeline for you!

addressed.

spackbot-app bot added defaults tests General test capability(ies) workflow gitlab Issues related to gitlab integration labels Aug 19, 2021

alalazo requested a review from scottwittenburg August 19, 2021 13:33

scottwittenburg reviewed Aug 19, 2021

View reviewed changes

alalazo requested review from becker33 and tgamblin August 19, 2021 16:27

becker33 requested changes Aug 19, 2021

View reviewed changes

alalazo mentioned this pull request Aug 20, 2021

concretizer: reuse existing packages #25310

Merged

8 tasks

spackbot-app bot added the environments label Aug 23, 2021

alalazo force-pushed the features/make_clingo_default branch from cf4ccb8 to 9924ab5 Compare August 23, 2021 15:33

becker33 previously requested changes Aug 23, 2021

View reviewed changes

lib/spack/spack/bootstrap.py Outdated Show resolved Hide resolved

alalazo mentioned this pull request Aug 23, 2021

Pipelines failing if two specs have same dag hash, but different full hashes #25563

Closed

3 tasks

tldahlgren requested changes Aug 23, 2021

View reviewed changes

lib/spack/spack/environment.py Show resolved Hide resolved

spackbot-app bot added conflicts update-package dependencies new-version patch labels Aug 23, 2021

alalazo marked this pull request as ready for review August 24, 2021 10:47

haampie reviewed Aug 24, 2021

View reviewed changes

This was referenced Aug 25, 2021

Regression test for version selection with preferences #25602

Merged

environment: match concrete specs only if they have the same build hash #25575

Merged

alalazo force-pushed the features/make_clingo_default branch from 5baed16 to 067c446 Compare August 27, 2021 12:12

tgamblin requested review from becker33 and tldahlgren September 11, 2021 00:42

tldahlgren previously approved these changes Sep 13, 2021

View reviewed changes

tgamblin mentioned this pull request Sep 13, 2021

Upcoming high-impact changes (perma-pin) #24223

Closed

becker33 reviewed Sep 13, 2021

View reviewed changes

alalazo added 9 commits September 13, 2021 13:41

Make clingo the default solver

2ecb657

- [x] Change defaults/config.yaml - [x] Add a fix for bootstrapping patchelf from sources if compilers.yaml is empty - [x] Make SPACK_TEST_SOLVER=clingo the default for unit-tests

Explicitly bootstrap before unit tests in centos6 and ubi

d0a1948

Update the default image, check if it makes a difference for clingo

22f9063

config.yaml: update the 'concretizer' comment

97cd8ef

Strengthen a bit initialization of compilers

e9a4422

Simplify code to find compilers if none is there

c7582c3

Fix e4s stack build with clingo

ca14888

Remove preferences that may direct the solver towards libtool@develop Add a conflict to numactl

libtool: deprecate version 2.4.2

86ec71d

tgamblin dismissed tldahlgren’s stale review via 86ec71d September 13, 2021 20:49

tgamblin force-pushed the features/make_clingo_default branch from 4a1b342 to 86ec71d Compare September 13, 2021 20:49

alalazo mentioned this pull request Sep 14, 2021

Update HPX package #25775

Merged

tgamblin approved these changes Sep 14, 2021

View reviewed changes

tgamblin merged commit c52426e into spack:develop Sep 15, 2021

alalazo deleted the features/make_clingo_default branch September 15, 2021 06:02

alalazo mentioned this pull request Sep 16, 2021

Concretizer dumps debug state instead of printing conflict messgae #25983

Open

3 tasks

sethrj mentioned this pull request Oct 3, 2021

pipelines: Change x86_64 E4S stack to use clingo concretizer #25287

Closed

Conversation

alalazo commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Aug 19, 2021

Uh oh!

spackbot-app bot commented Aug 19, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alalazo commented Aug 24, 2021

Uh oh!

haampie Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Aug 28, 2021

Uh oh!

spackbot-app bot commented Aug 28, 2021

Uh oh!

scottwittenburg commented Sep 10, 2021

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Sep 14, 2021

Uh oh!

spackbot-app bot commented Sep 14, 2021

Uh oh!

alalazo commented Sep 14, 2021

Uh oh!

spackbot-app bot commented Sep 14, 2021

Uh oh!

alalazo commented Sep 14, 2021

Uh oh!

spackbot-app bot commented Sep 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alalazo commented Aug 19, 2021 •

edited

Loading

alalazo Aug 20, 2021 •

edited

Loading

haampie Aug 24, 2021 •

edited

Loading