Concretization preserves deptypes by tgamblin · Pull Request #2681 · spack/spack

tgamblin · 2016-12-25T22:15:24Z

Fixes #1754. Fixes #1457.

Fixes issues with hashing where deptypes from one dependency would be
overwritten by those of another dependency on the same package.
- Copying specs (Spec.copy(), Spec._dup()) did not handle deptypes
  correctly:
- Copying "flattened" dependencies, but in doing so it collapsed
  multiple dependency relationships into one, losing some edge
  information in the DAG.
This gets rid of the flat_dependencies_with_deptypes() method,
which doesn't really make sense, as it collapses edges. It's now
reverted the original flat_dependnecies().
traverse_with_deptypes() is now called traverse_edges(), which is
when edge information must be preserved.
This gets rid of the notion of "default deptypes" introduced in Bugfix/cmdline deptypes #2307.
- Initially created Specs now have empty deptypes instead of "defaults".
- Proper deptypes are added during normalization/concretization like
  everything else in the Spec class. Empty deptypes mean deptypes are
  not yet specified, and the spec is abstract.
Updated tests.

davydden

I can't comment on changes to the core, but generally it looks good to me. I see that there are also regression tests which hopefully prevent clash of deptypes in future. Thanks for fixing the bug! 👍

tgamblin · 2016-12-26T21:15:12Z

@davydden: can you approve if you can verify it fixes the bug?

davydden

indeed, fixes #1457

$ spack spec -l dealii@develop+python+mpi
...
wfnrinu                  ^[email protected]%[email protected] arch=darwin-sierra-x86_64
hkosw63                      ^[email protected]%[email protected] arch=darwin-sierra-x86_64

while

$ spack spec -l dealii@develop
wfnrinu                  ^[email protected]%[email protected] arch=darwin-sierra-x86_64
hkosw63                      ^[email protected]%[email protected] arch=darwin-sierra-x86_64

(this is with the patch which previously triggered the problem davydden@f4ca2e7)

citibeth · 2016-12-29T04:20:05Z

As long as specs are being modified... is this a good opportunity to fix #1622 as well?

alalazo

LGTM. It solves the issues and looks more readable than before. I left a couple of questions for minor refactoring that can be addressed in following PRs.

alalazo · 2016-12-29T12:29:19Z

lib/spack/spack/__init__.py

 from spack.build_systems.makefile import MakefilePackage
 from spack.build_systems.autotools import AutotoolsPackage
 from spack.build_systems.cmake import CMakePackage
+__all__ += ['Package', 'CMakePackage', 'AutotoolsPackage', 'MakefilePackage']


Side comment: how do you feel about a decorator to be used in the core to add names to spack.__all__ ? I was thinking of something like:

@export class CMakePackage: ....

If you like the idea I'll prepare a small PR for that.

I kind of like the fact that you can read one file (__init__.py) and see what is in the spack wildcard import. I think if you put decorators on this it would be less apparent...

alalazo · 2016-12-29T12:36:33Z

lib/spack/spack/spec.py

+        self.deptypes = deptypes
+        return changed

    def copy(self):


Another side question: is it worth to implementing __copy__ and __deepcopy__ instead of using this method? If we conform to the copy module protocol, then we gain the possibility to seamlessly copy e.g. list of Specs or dicts of Specs, etc.

TBH I didn't know about __copy__ and __deepcopy__. But that sounds good to me! There is one complication.

If you notice, Spec has a _dup method, which does a copy in place. The copy() method currently uses that. It is used when you call .concretize() on Spec('mpi'), you want it to become mpich or openmpi or whatever.

I have thought for a while that an immutable implementation of Spec might be a better way to go, but it would be a major rework, and I have no evidence to quantify whether it would be a win performance-wise or not. On the one hand, the new concretizer will be iterating through a large space of Specs, and a completely immutable representation may hurt when we have to search a large space. On the other hand, I think it would simplify the implementation of the concretizer and Spec hashing, and the protocol in #1821 and #1875. So I guess my answer is let's open that can of worms when we get to it, which might be pretty soon here.

citibeth · 2016-12-29T14:13:24Z

In lib/spack/spack/__init__.py <#2681 (review)>: > from spack.build_systems.makefile import MakefilePackage from spack.build_systems.autotools import AutotoolsPackage from spack.build_systems.cmake import CMakePackage +__all__ += ['Package', 'CMakePackage', 'AutotoolsPackage', 'MakefilePackage'] Side comment: how do you feel about a decorator to be used in the core to add names to spack.__all__ ? I was thinking of something like: @exportclass CMakePackage: .... If you like the idea I'll prepare a small PR for that.

I think it's clever, but ultimately makes the code more obscure because it's no longer standard Python. Only worth it if MANY things are in `__all__` (with the definition of "many" left fuzzy). If it's going to be done, please be more descriptive with the decorator; like call it `add_to_all`.

adamjstewart · 2016-12-29T16:26:21Z

Just want to make sure I understand how things work now. Let's say package A depends on both B and C, and both B and C depend on package X. Now let's say the two dependencies on X are of different deptype. This looks something like:

![Alt text](http://g.gravizo.com/svg?
digraph G {
A -> B [label="[b,l]"];
B -> X [label="[b,l]"];
A -> C [label="[b,l]"];
C -> X [label="[b]"];
}
)

P.S. I've never used graphviz before, so if someone can fix this graph that would be great.

Do B and C maintain their own separate deptypes or are they flattened into a single deptype? The thing I'm worried about is building B and building C, then building A and finding it rebuilding B or C because the hash didn't include one of the dependencies. For example, would it rebuild C with a [b,l] dependency on X?

Another question. Let's take the same example as before, and let's say when B depends on X, the deptype is left unspecified. When C depends on X, the deptype is build. Now that there are no default dependencies and the spec is abstract, will the unspecified deptype be overwritten by build?

tgamblin · 2016-12-29T21:31:09Z

@adamjstewart:

Do B and C maintain their own separate deptypes or are they flattened into a single deptype?

Currently there are places in the code where B -> X and C-> X are flattened into a single deptype for X. This PR fixes that so that B -> X always has the deptypes it's supposed to per the B package and C -> X likewise has the deptypes it's supposed to per C's package.

The thing I'm worried about is building B and building C, then building A and finding it rebuilding B or C because the hash didn't include one of the dependencies. For example, would it rebuild C with a [b,l] dependency on X?

The hash of X isn't affected by the deptypes on X from B or C. Only the hashes of B and C are affected by that. So here, builds of A, B, and C can all depend on the same X. Also, If you build A, B, then C, and A doesn't do anything weird like depend on a special non-default version of B, then installing B and C should be no-ops (except to mark B and C as explicitly requested in the DB).

Another question. Let's take the same example as before, and let's say when B depends on X, the deptype is left unspecified. When C depends on X, the deptype is build. Now that there are no default dependencies and the spec is abstract, will the unspecified deptype be overwritten by build?

No, depends_on() without any explicit type arguments declares a ('build', 'link') dependency.

- Fixes issues with hashing where deptypes from one dependency would be overwritten by those of another dependency on the same package. - Copying specs (`Spec.copy()`, `Spec._dup()`) did not handle deptypes correctly: - Copying "flattened" dependencies, but in doing so it collapsed multiple dependency relationships into one, losing some edge information in the DAG. - This gets rid of the `flat_dependencies_with_deptypes()` method, which doesn't really make sense, as it collapses edges. It's now reverted the original `flat_dependnecies()`. - `traverse_with_deptypes()` is now called `traverse_edges()`, which is when edge information must be preserved. - This gets rid of the notion of "default deptypes" introduced in #2307. - Initially created Specs now have empty deptypes instead of "defaults". - Proper deptypes are added during normalization/concretization like everything else in the Spec class. Empty deptypes mean deptypes are not yet specified, and the spec is abstract. - Updated tests.

tgamblin requested review from adamjstewart, alalazo, becker33 and davydden December 25, 2016 22:15

tgamblin changed the title ~~Bugfix/concretize preserve deptypes~~ Bugfix: Concretize preserves deptypes Dec 25, 2016

tgamblin changed the title ~~Bugfix: Concretize preserves deptypes~~ Concretize preserves deptypes Dec 25, 2016

tgamblin added concretization dependencies specs hashes labels Dec 25, 2016

tgamblin requested a review from mathstuf December 25, 2016 22:34

tgamblin force-pushed the bugfix/concretize-preserve-deptypes branch 3 times, most recently from 19e8204 to 0593ab9 Compare December 26, 2016 02:24

davydden reviewed Dec 26, 2016

View reviewed changes

davydden approved these changes Dec 26, 2016

View reviewed changes

tgamblin mentioned this pull request Dec 28, 2016

Directive inheritance: laziness for the win #2623

Merged

3 tasks

alalazo approved these changes Dec 29, 2016

View reviewed changes

tgamblin added 5 commits December 29, 2016 13:34

clean up imports in spack.__init__.py

9d5c9b9

Add some more tests for deptypes.

5dda983

flake8 fixes.

08289c9

Fix bug in spack graph.

c52d6fe

tgamblin force-pushed the bugfix/concretize-preserve-deptypes branch from 0b37658 to c52d6fe Compare December 29, 2016 22:07

tgamblin changed the title ~~Concretize preserves deptypes~~ Concretization preserves deptypes Dec 29, 2016

tgamblin merged commit 5fbab1f into develop Dec 29, 2016

tgamblin mentioned this pull request Dec 30, 2016

Fix Python issue with build failures; Add test case for build failures. #2687

Merged

2 tasks

tgamblin deleted the bugfix/concretize-preserve-deptypes branch December 31, 2016 04:58

adamjstewart mentioned this pull request Jan 5, 2017

spack activate does not activate dependencies #2760

Closed

tgamblin mentioned this pull request Apr 6, 2017

Spack Bug : Adding Explicit Dependency to Package Can Make it Unusable by Dependents #1559

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Concretization preserves deptypes#2681

Concretization preserves deptypes#2681
tgamblin merged 5 commits intodevelopfrom
bugfix/concretize-preserve-deptypes

tgamblin commented Dec 25, 2016 •

edited

Loading

Uh oh!

davydden left a comment

Uh oh!

tgamblin commented Dec 26, 2016

Uh oh!

davydden left a comment

Uh oh!

citibeth commented Dec 29, 2016

Uh oh!

alalazo left a comment

Uh oh!

alalazo Dec 29, 2016

Uh oh!

tgamblin Dec 29, 2016

Uh oh!

alalazo Dec 29, 2016

Uh oh!

tgamblin Dec 29, 2016

Uh oh!

citibeth commented Dec 29, 2016 via email

Uh oh!

adamjstewart commented Dec 29, 2016 •

edited by tgamblin

Loading

Uh oh!

tgamblin commented Dec 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

tgamblin commented Dec 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davydden left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Dec 26, 2016

Uh oh!

davydden left a comment

Choose a reason for hiding this comment

Uh oh!

citibeth commented Dec 29, 2016

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

tgamblin Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

alalazo Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

tgamblin Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

citibeth commented Dec 29, 2016 via email

Uh oh!

adamjstewart commented Dec 29, 2016 • edited by tgamblin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Dec 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tgamblin commented Dec 25, 2016 •

edited

Loading

adamjstewart commented Dec 29, 2016 •

edited by tgamblin

Loading