Fix Bug merging configs. by citibeth · Pull Request #7925 · spack/spack

citibeth · 2018-04-26T19:11:05Z

@tgamblin Can this be fast-tracked? It is a small change, and fixes a definite bug #7924. Previously, copy.copy() was not doing a correct deep copy of syaml_dict.

copy.copy() was not doing a correct deep copy of syaml_dict.

alalazo · 2018-04-26T19:15:26Z

lib/spack/spack/config.py

        for sk, sv in iteritems(source):
            if override(sk) or sk not in dest:
                # if sk ended with ::, or if it's new, completely override
-                dest[sk] = copy.copy(sv)


I didn't check the logic change that this PR brings, but if we need a deep copy, why aren't we using copy.deepcopy?

citibeth · 2018-04-26T19:30:38Z

Because I didn't know it existed. Let me try that, I'll report back...

…

On Thu, Apr 26, 2018 at 3:15 PM, Massimiliano Culpo < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/spack/spack/config.py: > @@ -363,15 +375,15 @@ def they_are(t): for sk, sv in iteritems(source): if override(sk) or sk not in dest: # if sk ended with ::, or if it's new, completely override - dest[sk] = copy.copy(sv) I didn't check the logic, but if we need a deep copy, why aren't we using copy.deepcopy? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7925 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cdwpWhUhYogtPjIBftvXAWDMrLetNks5tshzQgaJpZM4Tm-uY> .

Replace homegrown deepcopy with standard `copy.deepcopy()`, which works just as well.

Add back `import copy`

citibeth · 2018-04-28T21:46:01Z

@alalazo @tgamblin OK this PR is ready to merge, and it fixes a serious config bug that affects concretization.

With bugfix:

$ spack --color always env twoway-dev spec ibmisc | egrep 'boost|ibmisc'
ibmisc
ibmisc@develop%gcc@4.9.3+blitz+boost build_type=RelWithDebInfo ~doc+everytrace+googletest+netcdf+proj+python+udunits2 arch=linux-centos7-x86_64 
    ^boost@1.66.0%gcc@4.9.3+atomic+chrono~clanglibcpp+date_time~debug+exception+filesystem~graph~icu+iostreams+locale+log+math~mpi+multithreaded patches=2ab6c72d03dec6a4ae20220a9dfd5c8c572c5294252155b85c6874d97c323199 +program_options~python+random+regex+serialization+shared+signals~singlethreaded+system~taggedlayout+test+thread+timer~versionedlayout+wave arch=linux-centos7-x86_64

This is correct. In my various config scopes, I have specified the ibmisc@develop, but no specific version for boost. Without bugfix:

$ spack --color always env twoway-dev spec ibmisc | egrep 'boost|ibmisc'
ibmisc
ibmisc@develop%gcc@4.9.3+blitz+boost build_type=RelWithDebInfo ~doc+everytrace+googletest+netcdf+proj+python+udunits2 arch=linux-centos7-x86_64 
    ^boost@develop%gcc@4.9.3+atomic+chrono~clanglibcpp+date_time~debug+exception+filesystem~graph~icu+iostreams+locale+log+math~mpi+multithreaded patches=2ab6c72d03dec6a4ae20220a9dfd5c8c572c5294252155b85c6874d97c323199 +program_options~python+random+regex+serialization+shared+signals~singlethreaded+system~taggedlayout+test+thread+timer~versionedlayout+wave arch=linux-centos7-x86_64

This is wrong... I get boost@develop, even though my various packages.yaml only specify develop for ibmisc. The lack of deep copy was causing aliasing of YAML dicts; which would cause unpredictable results in the answer. In this case, the merge was producing all: +develop, so the @develop version was chosen for any package that provided it.

scheibelp · 2018-04-30T18:24:40Z

b6878b7 appears to copy this but doesnt remove _copy - any reason why not?

Also, a unit test would be excellent so this doesn't happen again in 6 months. Let me know if you'd like me to do that and I can give it a go; in that case more context for the problem - e.g. the conflicting config files - would be excellent.

citibeth · 2018-04-30T18:37:41Z

b6878b7 appears to copy this but doesnt remove _copy - any reason why not?

Huh? Looks to me like it removes _copy().

scheibelp · 2018-04-30T18:41:20Z

b6878b7 appears to copy this but doesnt remove _copy - any reason why not?

Huh? Looks to me like it removes _copy().

I meant to say: b6878b7 removes _copy, but this PR does not. I'm curious why.

citibeth · 2018-04-30T18:43:52Z

Also, a unit test would be excellent so this doesn't happen again in 6 months. Let me know if you'd like me to do that and I can give it a go; in that case more context for the problem - e.g. the conflicting config files - would be excellent.

I really appreciate your willingness to write a unit test. Here are the basics, not sure the best way to turn that into a unit test.

I'm using a Spack Enviroment with the following env.yaml:

env:
    configs:
        - '{HOME}/spscopes/twoway'
        - '{HOME}/spscopes/develop'            # Highest precedence

See attached for twoway/package.yaml and develop/packages.yaml.

develop-packages.yaml.txt
twoway-packages.yaml.txt

Next, I added the following debug printout to config.py, around line 428:

    # no config files -- empty config.
    if section not in merged_section:
        return {}

+    if section == 'packages':
+       print('pppppackages', merged_section[section])
+        sys.exit(0)

    # take the top key off before returning.
    return merged_section[section]

With this PR merged, when I run anything (say spack env myenv spec emacs) I get this correct result:

('pppppackages', {'all': {'compiler': ['gcc', 'intel', 'pgi', 'clang', 'xl', 'nag'], 'providers': {'awk': ['gawk'], 'blas': ['openblas'], 'daal': ['intel-daal'], 'elf': ['elfutils'], 'gl': ['mesa', 'opengl'], 'glu': ['mesa-glu', 'openglu'], 'golang': ['gcc'], 'ipp': ['intel-ipp'], 'java': ['jdk'], 'lapack': ['openblas'], 'mkl': ['intel-mkl'], 'mpe': ['mpe2'], 'mpi': ['openmpi', 'mpich'], 'opencl': ['pocl'], 'openfoam': ['openfoam-com', 'openfoam-org', 'foam-extend'], 'pil': ['py-pillow'], 'pkgconfig': ['pkgconf', 'pkg-config'], 'scalapack': ['netlib-scalapack'], 'szip': ['libszip', 'libaec'], 'tbb': ['intel-tbb'], 'jpeg': ['libjpeg-turbo', 'libjpeg']}, 'paths': {}, 'modules': {}, 'buildable': True, 'version': []}, 'foo-a': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'pism': {'variants': ['~python', '+examples'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'version': ['dev'], 'compiler': []}, 'icebin': {'variants': ['+coupler', '+modele', '+pism'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'version': ['develop'], 'compiler': []}, 'modele': {'variants': ['+debug', '+mods', '+everytrace', '+pnetcdf', '+icebin', '+mpi', '+model'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'version': ['landice'], 'compiler': []}, 'ibmisc': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'py-giss': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'modele-control': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'everytrace': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}})

When I run without the PR merged, I get this incorrect result:

('pppppackages', {'all': {'compiler': ['gcc', 'intel', 'pgi', 'clang', 'xl', 'nag'], 'providers': {'awk': ['gawk'], 'blas': ['openblas'], 'daal': ['intel-daal'], 'elf': ['elfutils'], 'gl': ['mesa', 'opengl'], 'glu': ['mesa-glu', 'openglu'], 'golang': ['gcc'], 'ipp': ['intel-ipp'], 'java': ['jdk'], 'lapack': ['openblas'], 'mkl': ['intel-mkl'], 'mpe': ['mpe2'], 'mpi': ['openmpi', 'mpich'], 'opencl': ['pocl'], 'openfoam': ['openfoam-com', 'openfoam-org', 'foam-extend'], 'pil': ['py-pillow'], 'pkgconfig': ['pkgconf', 'pkg-config'], 'scalapack': ['netlib-scalapack'], 'szip': ['libszip', 'libaec'], 'tbb': ['intel-tbb'], 'jpeg': ['libjpeg-turbo', 'libjpeg']}, 'paths': {}, 'modules': {}, 'buildable': True, 'version': ['landice', 'dev', 'develop']}, 'foo-a': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'pism': {'variants': ['~python', '+examples'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'version': ['landice', 'dev', 'develop'], 'compiler': []}, 'icebin': {'variants': ['+coupler', '+modele', '+pism'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'version': ['landice', 'dev', 'develop'], 'compiler': []}, 'modele': {'variants': ['+debug', '+mods', '+everytrace', '+pnetcdf', '+icebin', '+mpi', '+model'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'version': ['landice', 'dev', 'develop'], 'compiler': []}, 'ibmisc': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'py-giss': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'modele-control': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}, 'everytrace': {'version': ['develop'], 'paths': {}, 'providers': {}, 'modules': {}, 'buildable': True, 'compiler': []}})

There is no simple test for this issue because it's an aliasing problem (eg: memory corruption).

citibeth · 2018-04-30T18:47:24Z

I meant to say: b6878b7 removes _copy, but this PR does not. I'm curious why.

I added _copy() in this PR. Then @alalazo mentioned it was not needed, and I removed it from the PR. Hence now the PR as a whole doesn't add or remove _copy().

citibeth · 2018-05-01T20:17:54Z

Replaced by #7959

Fixes bug merging configs.

ac6ec3f

copy.copy() was not doing a correct deep copy of syaml_dict.

citibeth added the bug Something isn't working label Apr 26, 2018

citibeth requested a review from tgamblin April 26, 2018 19:11

citibeth mentioned this pull request Apr 26, 2018

_merge_yaml() doesn't work #7924

Closed

alalazo reviewed Apr 26, 2018

View reviewed changes

flake8

f538931

adamjstewart added the configuration label Apr 26, 2018

Elizabeth Fischer added 2 commits April 28, 2018 17:38

Update config.py

a1f66f1

Replace homegrown deepcopy with standard `copy.deepcopy()`, which works just as well.

Update config.py

42aa727

Add back `import copy`

citibeth requested a review from alalazo April 28, 2018 21:40

citibeth added the ready label Apr 28, 2018

scheibelp self-assigned this May 1, 2018

scheibelp mentioned this pull request May 1, 2018

Fix initialization of defaults in yaml schema validation #7959

Merged

citibeth closed this May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bug merging configs.#7925

Fix Bug merging configs.#7925
citibeth wants to merge 4 commits intospack:developfrom
citibeth:efischer/180426-FixConfigMerging

citibeth commented Apr 26, 2018

Uh oh!

alalazo Apr 26, 2018 •

edited

Loading

Uh oh!

citibeth commented Apr 26, 2018 via email

Uh oh!

citibeth commented Apr 28, 2018

Uh oh!

scheibelp commented Apr 30, 2018

Uh oh!

citibeth commented Apr 30, 2018

Uh oh!

scheibelp commented Apr 30, 2018

Uh oh!

citibeth commented Apr 30, 2018

Uh oh!

citibeth commented Apr 30, 2018

Uh oh!

citibeth commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

citibeth commented Apr 26, 2018

Uh oh!

alalazo Apr 26, 2018 • 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!

citibeth commented Apr 26, 2018 via email

Uh oh!

citibeth commented Apr 28, 2018

Uh oh!

scheibelp commented Apr 30, 2018

Uh oh!

citibeth commented Apr 30, 2018

Uh oh!

scheibelp commented Apr 30, 2018

Uh oh!

citibeth commented Apr 30, 2018

Uh oh!

citibeth commented Apr 30, 2018

Uh oh!

citibeth commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alalazo Apr 26, 2018 •

edited

Loading