Fix Python build for AOCC compiler by amd-toolchain-support · Pull Request #28708 · spack/spack

amd-toolchain-support · 2022-02-01T13:33:23Z

Disable new varient ensurepip for AOCC compiler to prevent following error while building Python in spack environment:

IndexError: string index out of range

Python recently made ensurepip as default varient for all python versions, it was observed that AOCC-3.2.0 compiler was failing while compiling ensurepip module in Python code

Below is the associated commit which mandate ensurepip as default varient :
commit ffb3f3a
Author: Wouter Deconinck wdconinc@gmail.com
Date: Sat Jan 22 21:55:56 2022 -0600

Python: ensurepip install instead of upgrade (#28553)

Fixes #29115

…error while building Python IndexError: string index out of range python recently made ensurepip as default varient for all python versions, it was observed that AOCC-3.2.0 compiler was failing while compiling ensurepip module in Python code

adamjstewart · 2022-02-01T20:20:37Z

IndexError: string index out of range

Can you give more context? I would love to fix this if possible.

I think a better solution would be to add:

conflicts('+ensurepip', when='%aocc')

amd-toolchain-support · 2022-02-02T16:53:17Z

Can you give more context? I would love to fix this if possible.
Investigation is still on, product team will provide fix by next release.

amd-toolchain-support · 2022-02-02T17:15:18Z

With conflict change, I noticed Concretizer uses 'gcc' instead of 'aocc' compiler for amdblis product

--snip--
$ spack spec -IL amdblis%aocc
Input spec
--------------------------------
 -   amdblis%aocc

Concretized
--------------------------------
[+]  qfqke5wwgsmcympk6s6abxpsvk42fwt5  amdblis@3.1%aocc@3.2.0+blas+cblas~ilp64+shared+static threads=none arch=linux-ubuntu20.04-zen2
[+]  p4ibffbwogr4ugaef5foapw6rkf6yetg      ^python@3.9.10%gcc@9.3.0+bz2+ctypes+dbm~debug+ensurepip+libxml2+lzma~nis~optimizations+pic+pyexpat+pythoncmd+readline+shared+sqlite3+ssl~tix~tkinter~ucs4+uuid+zlib patches=0d98e93189bc278fbc37a50ed7f183bd8aaf249a8e1670a465f0db6bb4f8cf87,4c2457325f2b608b1b6a2c63087df8c26e07db3e3d493caf36a56f0ecf6fb768,f2fd060afc4b4618fe8104c4c5d771f36dc55b1db5a4623785a4ea707ec72fb4 arch=linux-ubuntu20.04-zen2
[+]  gk3mz66h7c2asfd3zamm3bmnp73bk46f          ^bzip2@1.0.8%gcc@9.3.0~debug~pic+shared arch=linux-ubuntu20.04-zen2
[+]  vtnqdp4ihklrznse2qsskmruz7krvgkw              ^diffutils@3.8%gcc@9.3.0 arch=linux-ubuntu20.04-zen2
[+]  6cvwkbye3jn7wlcmzbvi5po3amzt7gee                  ^libiconv@1.16%gcc@9.3.0 libs=shared,static arch=linux-ubuntu20.04-zen2
[+]  4dnbunya535sscm2tbodnxuvttfzvh5f          ^expat@2.4.3%gcc@9.3.0+libbsd arch=linux-ubuntu20.04-zen2
[+]  2m2xjhv6jghghk25b4esiwopnc7gfmff              ^libbsd@0.11.3%gcc@9.3.0 arch=linux-ubuntu20.04-zen2
[+]  fz6desqy5si22fxs6kmhk7l6gwyca35n                  ^libmd@1.0.3%gcc@9.3.0 arch=linux-ubuntu20.04-zen2
[+]  ha5rdnyagdmd6poh7yzq2kpfw3t4ulfc          ^gdbm@1.19%gcc@9.3.0 arch=linux-ubuntu20.04-zen2
[+]  a2i6r6bvyr2oxuctgghpj2rjofhuqgsg              ^readline@8.1%gcc@9.3.0 arch=linux-ubuntu20.04-zen2
[+]  s76nhbfwlj74krqyz2vvniq2ufbxqkq5                  ^ncurses@6.2%gcc@9.3.0~symlinks+termlib abi=none arch=linux-ubuntu20.04-zen2
[+]  izybkdnqi2scuqh5uzokvmvr62x2x7os                      ^pkgconf@1.8.0%gcc@9.3.0 arch=linux-ubuntu20.04-zen2
[+]  a7jjrisnzyslxvd6ihihhs52r5v2bk6r          ^gettext@0.21%gcc@9.3.0+bzip2+curses+git~libunistring+libxml2+tar+xz arch=linux-ubuntu20.04-zen2
[+]  pahfvvcceu4wyhwwa64umdfcrxowat2u              ^libxml2@2.9.12%gcc@9.3.0~python arch=linux-ubuntu20.04-zen2
[+]  74ydxri5j5u7o4qxazi55uuwl3y6nom4                  ^xz@5.2.5%gcc@9.3.0~pic libs=shared,static arch=linux-ubuntu20.04-zen2
[+]  whj3n7mok67wqtm5dofead56i6krevxc                  ^zlib@1.2.11%gcc@9.3.0+optimize+pic+shared arch=linux-ubuntu20.04-zen2
[+]  2thrjwrd7mj36d2pylkviidlab5rl6si              ^tar@1.34%gcc@9.3.0 arch=linux-ubuntu20.04-zen2
[+]  aw6hgeukakt5wpkqeghsupn5fdqo66uh          ^libffi@3.3%gcc@9.3.0 patches=26f26c6f29a7ce9bf370ad3ab2610f99365b4bdd7b82e7c31df41a3370d685c0 arch=linux-ubuntu20.04-zen2
[+]  23imk2rsftvme5mwrkr6wdvhoecqq2rk          ^openssl@1.1.1m%gcc@9.3.0~docs certs=system arch=linux-ubuntu20.04-zen2
[+]  pc3phvpaqm5da5dggnwyc3jwqelpzt7d              ^perl@5.34.0%gcc@9.3.0+cpanm+shared+threads arch=linux-ubuntu20.04-zen2
[+]  saforstflagtfktlvcp47xn2po4kvu4j                  ^berkeley-db@18.1.40%gcc@9.3.0+cxx~docs+stl patches=b231fcc4d5cff05e5c3a4814f6a5af0e9a966428dc2176540d2c05aff41de522 arch=linux-ubuntu20.04-zen2
[+]  qv4twbyh5vnzd6zsd2b3mcgy7rb577w2          ^sqlite@3.37.1%gcc@9.3.0+column_metadata+dynamic_extensions+fts~functions+rtree arch=linux-ubuntu20.04-zen2
[+]  24qreesevr22bklibvskd6xyvd5gyu4p          ^util-linux-uuid@2.36.2%gcc@9.3.0 arch=linux-ubuntu20.04-zen2

--snip--

Could you please help to explain the reason?

adamjstewart · 2022-02-02T17:18:33Z

I don't see amdblis in that DAG

amd-toolchain-support · 2022-02-02T19:06:39Z

DAG

Accidently I copied python spec output.
later I updated output of 'spack spec -IL amdblis%aocc'

adamjstewart · 2022-02-02T19:37:27Z

Thanks, that's because our concretizer currently prioritizes the number of non-default variants above compiler mismatches. @alalazo I wonder if it makes sense to have greater priority for compiler mismatches than we currently have.

alalazo · 2022-02-03T10:04:34Z

I wonder if it makes sense to have greater priority for compiler mismatches than we currently have.

It's hard to think at all possible combinations, but I am pretty sure that will cause a lot of other weird issues e.g. variants being flipped so that compilers can be matched etc.

amd-toolchain-support · 2022-02-04T18:19:54Z

In the current situation where I understand:

Concretizer prioritizes the number of non-default variants above compiler mismatches
If we try to Increase the priority for compiler mismatches will call of issues

Then I would request to consider my initial change request i.e.,

--snip--
$ git diff
diff --git a/var/spack/repos/builtin/packages/python/package.py b/var/spack/repos/builtin/packages/python/package.py
index a9103aca6d..7fb53abee7 100644
--- a/var/spack/repos/builtin/packages/python/package.py
+++ b/var/spack/repos/builtin/packages/python/package.py
@@ -494,7 +494,10 @@ def configure_args(self):

         if spec.satisfies('@2.7.9:2,3.4:'):
             if '+ensurepip' in spec:
-                config_args.append('--with-ensurepip=install')
+                if '%aocc@3.2.0' in spec:
+                    config_args.append('--without-ensurepip')
+                else:
+                    config_args.append('--with-ensurepip=install')
             else:
                 config_args.append('--without-ensurepip')

$
--snip--

adamjstewart · 2022-02-04T18:57:28Z

The problem with the initial change request is that python+ensurepip%aocc and python~ensurepip%aocc will result in two different installations with different hashes despite the installations themselves being identical.

amd-toolchain-support · 2022-02-18T08:41:58Z

@adamjstewart

As the impact of AOCC compiler issue is huge on AMD libraries and associated applications can we restore the python ensurepip to default off state like earlier ?

adamjstewart · 2022-02-18T16:53:34Z

@becker33 is it possible to change a variant default based on compiler? Does this work:

variant('ensurepip', default=True)
variant('ensurepip', default=False, when='%aocc')

It's hard to think at all possible combinations, but I am pretty sure that will cause a lot of other weird issues e.g. variants being flipped so that compilers can be matched etc.

@alalazo yes, this is exactly the behavior that we want in this situation. I would assume that ABI compatibility and compiler consistency would be our top priority. I do want to make sure that we avoid changing the default compiler and concretizing everything with GCC instead of AOCC though.

Overall, I'm still leaning towards the current solution (just waiting for that newline to be removed before merging). There are far more users who use pip than there are users who use AOCC. For users who use AOCC, they can always use:

$ spack install python~ensurepip %aocc

or add:

packages:
  python:
    variants: ~ensurepip

to their packages.yaml if building with GCC is an issue. Of course, if we can modify the concretization strategy to prefer compiler consistency or set the variant default based on compiler that would be the best solution. But changing the variant default due to a single compiler doesn't seem like a good solution.

zzzoom · 2022-02-22T18:54:01Z

Removing ensurepip allows the package installation to complete but the resulting python is broken.

$ spack load python%aocc@3.2.0
$ python 
Python 3.10.2 (main, Feb 22 2022, 00:39:47) [Clang 13.0.0 (CLANG: AOCC_3.2.0-Build#128 2021_11_12)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 
$ python -m test
Traceback (most recent call last):
  File "/home/bc/spack/opt/spack/linux-rocky8-zen2/aocc-3.2.0/python-3.10.2-wf7l7fn74wc7ws57hszudj73xrqsdawq/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/home/bc/spack/opt/spack/linux-rocky8-zen2/aocc-3.2.0/python-3.10.2-wf7l7fn74wc7ws57hszudj73xrqsdawq/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/home/bc/spack/opt/spack/linux-rocky8-zen2/aocc-3.2.0/python-3.10.2-wf7l7fn74wc7ws57hszudj73xrqsdawq/lib/python3.10/test/__main__.py", line 1, in <module>
    from test.libregrtest import main
  File "/home/bc/spack/opt/spack/linux-rocky8-zen2/aocc-3.2.0/python-3.10.2-wf7l7fn74wc7ws57hszudj73xrqsdawq/lib/python3.10/test/libregrtest/__init__.py", line 1, in <module>
    from test.libregrtest.cmdline import _parse_args, RESOURCE_NAMES, ALL_RESOURCES
  File "/home/bc/spack/opt/spack/linux-rocky8-zen2/aocc-3.2.0/python-3.10.2-wf7l7fn74wc7ws57hszudj73xrqsdawq/lib/python3.10/test/libregrtest/cmdline.py", line 4, in <module>
    from test import support
  File "/home/bc/spack/opt/spack/linux-rocky8-zen2/aocc-3.2.0/python-3.10.2-wf7l7fn74wc7ws57hszudj73xrqsdawq/lib/python3.10/test/support/__init__.py", line 18, in <module>
    from .testresult import get_test_runner
IndexError: string index out of range

…6 April 2022 09:39:56 AM EST

spack#29115 spack#28708

vkallesh · 2022-04-06T17:28:54Z

Please note, in the new commit we have removed below line:
conflicts('+ensurepip', when='%aocc')

and added following section:
if self.spec.satisfies('%aocc@3.2.0', strict=True):
if name == 'cflags':
flags.append(' -mllvm -enable-gvn-sink=true ')

as we found above flag is not related to the bug that's causing the failure, but it runs some additional passes and modifies the code and perhaps the buggy code pattern is disturbed hence the error is suppressed.

zzzoom · 2022-04-06T19:49:40Z

On our system, the proposed fix worked for python@3.9.10 %aocc@3.2.0 but it didn't work for python@3.10.3 %aocc@3.2.0.

…m enable-gvn-sink=true" With the new flag python build with AOCC compiler is working fine for both python@3.9.0 and python@3.10.3 versions.

amd-toolchain-support · 2022-04-08T08:31:00Z

On our system, the proposed fix worked for python@3.9.10 %aocc@3.2.0 but it didn't work for python@3.10.3 %aocc@3.2.0.

New commit done:

Changes:
Updated "-mllvm -disable-indvar-simplify=true" flag instead of "-mllvm enable-gvn-sink=true" flag

With the new flag python build with AOCC compiler is working fine for both python@3.9.0 and python@3.10.3 versions.

zzzoom · 2022-04-08T23:39:01Z

Tried 3.10.4, 3.9.12, 3.8.13, 3.7.13, 3.6.15 and 2.7.17 with the new flags, all of them built successfully and passed all or most of the python test suite.

vkallesh · 2022-04-09T03:02:31Z

Tried 3.10.4, 3.9.12, 3.8.13, 3.7.13, 3.6.15 and 2.7.17 with the new flags, all of them built successfully and passed all or most of the python test suite.

Thanks for certifying the solution. This will help for quick approval.

… April 2022 08:47:24 AM IST

adamjstewart · 2022-04-09T20:15:48Z

+        # https://github.com/spack/spack/pull/28708
+        if self.spec.satisfies('%aocc@3.2.0', strict=True):
+            if name == 'cflags':
+                flags.extend(['-mllvm','-disable-indvar-simplify=true'])


Suggested change

flags.extend(['-mllvm','-disable-indvar-simplify=true'])

flags.extend(['-mllvm', '-disable-indvar-simplify=true'])

This should fix the style tests

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>

spackbot-app Bot added python update-package labels Feb 1, 2022

spackbot-app Bot requested review from adamjstewart, scheibelp, skosukhin and varioustoxins February 1, 2022 13:33

amd-toolchain-support added AMD aocc libraries labels Feb 1, 2022

adamjstewart self-assigned this Feb 1, 2022

Reviewers suggestion is incorporated

f2fb793

spackbot-app Bot added the conflicts label Feb 2, 2022

adamjstewart reviewed Feb 2, 2022

View reviewed changes

Comment thread var/spack/repos/builtin/packages/python/package.py Outdated

Removed empty line

2606831

adamjstewart mentioned this pull request Feb 22, 2022

Installation issue: python %aocc@3.2.0 #29115

Closed

4 tasks done

adamjstewart requested changes Feb 22, 2022

View reviewed changes

Comment thread var/spack/repos/builtin/packages/python/package.py Outdated

Merge changes from Spack community develop branch as on - Wednesday 0…

66a50a7

…6 April 2022 09:39:56 AM EST

Fix following issues for python with aocc%3.2.0:

f53a8d8

spack#29115 spack#28708

adamjstewart reviewed Apr 6, 2022

View reviewed changes

adamjstewart changed the title ~~Disable new varient ensurepip for AOCC compiler~~ Fix Python build for AOCC compiler Apr 6, 2022

Updated "-mllvm -disable-indvar-simplify=true" flag instead of "-mllv…

db3970a

…m enable-gvn-sink=true" With the new flag python build with AOCC compiler is working fine for both python@3.9.0 and python@3.10.3 versions.

amd-toolchain-support added 4 commits April 9, 2022 08:47

Merge changes from Spack community develop branch as on - Saturday 09…

c4b1fce

… April 2022 08:47:24 AM IST

Incorporated reviewers suggestions

99bb6cb

changed to right URL reference

49870eb

update cflags list from append to extend

bc69be2

adamjstewart previously approved these changes Apr 9, 2022

View reviewed changes

adamjstewart reviewed Apr 9, 2022

View reviewed changes

Update var/spack/repos/builtin/packages/python/package.py

a418b42

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>

amd-toolchain-support dismissed adamjstewart’s stale review via a418b42 April 11, 2022 03:14

adamjstewart approved these changes Apr 11, 2022

View reviewed changes

alalazo enabled auto-merge (squash) April 11, 2022 07:24

alalazo merged commit ae76834 into spack:develop Apr 11, 2022

joequant pushed a commit to hkphysics/spack that referenced this pull request Apr 17, 2022

Python: fix build with AOCC compiler (spack#28708)

ebbef4f

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>

	flags.extend(['-mllvm','-disable-indvar-simplify=true'])
	flags.extend(['-mllvm', '-disable-indvar-simplify=true'])

Conversation

amd-toolchain-support commented Feb 1, 2022 • edited by adamjstewart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Feb 1, 2022

Uh oh!

Uh oh!

amd-toolchain-support commented Feb 2, 2022

Uh oh!

amd-toolchain-support commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Feb 2, 2022

Uh oh!

amd-toolchain-support commented Feb 2, 2022

Uh oh!

adamjstewart commented Feb 2, 2022

Uh oh!

alalazo commented Feb 3, 2022

Uh oh!

amd-toolchain-support commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Feb 4, 2022

Uh oh!

amd-toolchain-support commented Feb 18, 2022

Uh oh!

adamjstewart commented Feb 18, 2022

Uh oh!

zzzoom commented Feb 22, 2022

Uh oh!

Uh oh!

vkallesh commented Apr 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zzzoom commented Apr 6, 2022

Uh oh!

amd-toolchain-support commented Apr 8, 2022

Uh oh!

zzzoom commented Apr 8, 2022

Uh oh!

vkallesh commented Apr 9, 2022

Uh oh!

adamjstewart Apr 9, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amd-toolchain-support commented Feb 1, 2022 •

edited by adamjstewart

Loading

amd-toolchain-support commented Feb 2, 2022 •

edited

Loading

amd-toolchain-support commented Feb 4, 2022 •

edited

Loading