Allow combinatorial projections in views by becker33 · Pull Request #9679 · spack/spack

becker33 · 2018-10-30T22:15:43Z

Allow views to specify projections more complicated than merging every package into a shared prefix. This will allow sites to configure a view for the way they want to present packages to their users.

Create a yaml schema for specifying projections of packages into a view (based on spec.format strings)
Formalize the schema and check for adherence
Ensure that extensions work with combinatorial views
Update spec.format to allow dependency information in format strings
Add testing for combinatorial views
Update documentation for views with options for specifying projections

From the new documentation for views:

The default projection into a view is to link every package into the
root of the view. This can be changed through the projections.yaml
file in the view. The projection configuration file for a view located
at /my/view is stored in /my/view/.spack/projections.yaml.

When creating a view, the projection configuration file can also be
specified from the command line using the --projection-file option
to the spack view command.

The projections configuration file is a mapping of partial specs to
spec format strings, as shown in the example below.

projections:
    zlib: ${PACKAGE}-${VERSION}
    ^mpi: ${PACKAGE}-${VERSION}/${DEP:mpi:PACKAGE}-${DEP:mpi:VERSION}-${COMPILERNAME}-${COMPILERVER}
    all: ${PACKAGE}-${VERSION}/${COMPILERNAME}-${COMPILERVER}

The entries in the projections configuration file must all be either
specs or the keyword all. ~~For each spec, the projection used will
be the first non-all entry that the spec satisfies, or the projection for the
all entry if one exists and the spec satisfies no other entries and the keyword all is
satisfied by any spec. Given the example above, any spec satisfying
[email protected] will be linked into /my/view/zlib-1.2.8/, any spec
satisfying [email protected]+mpi %[email protected] ^[email protected] will be linked
into /my/view/hdf5-1.8.10/mvapich2-2.2-gcc-4.9.3, and any spec
satisfying [email protected]~mpi %[email protected] will be linked into
/my/view/hdf5-1.8.10/gcc-4.9.3.

If the keyword all does not appear in the projections
configuration file, any spec that does not satisfy any entry in the
file will be linked into the root of the view as in a single-prefix
view. Any entries that appear below the keyword all in the
projections configuration file will not be used, as all specs will use
the projection under all before reaching those entries.

scheibelp

Overall, I think the primary change that needs to be made here is that there are members of YamlFilesystemView which should no longer be accessible given the fact that the view no longer has a single root (namely .root and .extensions_layout). Those should be replaced with functions that take a Spec as a parameter.

Let me know if this is unclear or seems off-target.

lib/spack/docs/workflows.rst

lib/spack/spack/cmd/view.py

scheibelp · 2019-01-03T19:42:00Z

lib/spack/spack/filesystem_view.py

+    try:
+        with open(filename, "r") as f:
+            return spack.spec.Spec.from_yaml(f)
+    except IOError:


IOError includes errors like "file not found" as well as permission errors. This appears to have been consolidated from existing logic so that's not necessarily your choice, but IMO it's worthwhile to get more specific about the error.

It may also be worth creating a more generic utility function that, given a file name, opens it and applies a function to the result.

You're correct that this was already extant code, but I disagree that it's a misuse of IOError. We want a no-exception method for getting specs from files, so that the calling code can determine whether not getting a spec is an error or not.

I actually agree with Peter that this should be more specific -- why does it have to squash the IOError? Doing that means the calling code knows a lot less about what happened.

In this case, there is actually likely to be something wrong if from_yaml() fails, because the calling code got the name from os.listdir(), so you know the file should really exist. If it is corrupt or unreadable, the user likely wants to know, at least with a warning.

Note that from_yaml() can take a filename, so you don't need the open call here. would just inline this function and have the calling code determine what is worth catching...

lib/spack/docs/workflows.rst

lib/spack/spack/cmd/view.py

lib/spack/spack/filesystem_view.py

scheibelp · 2019-01-04T00:01:24Z

lib/spack/spack/filesystem_view.py

        """
        raise NotImplementedError

+    def get_projection_for_spec(self, spec):


I think this is fine but that any user of YamlFilesystemView that originally wanted to access view.root would now want to use this function. Therefore I think view.root should be replaced with view._root (i.e. that is now a private and internal detail of the view). Currently I don't think all build system implementations (e.g. Python and Aspell) are consistent with the other changes in this PR but making this change would help with that.

It seems that the check in the AspellDictPackage class is still a correct use of the view root to check whether the view is being used for an extension or externally.

I've updated the PythonPackage build system to use the new syntax to get its location within the view. Since python packages are always extensions and extensions are linked into combinatorial views using non-combinatorial sub-views, it doesn't actually make a difference, but I think relying on that would be too fragile.

I want to eliminate the possibility of getting it wrong by forcing the usage that is always correct. Basically I don't want future developers to have to reason like this (although I agree with your points).

I'm still not convinced that root shouldn't be something you're allowed to ask a view. But I don't think removing it will harm any of the current cases, so I'll make the change and we can always change it back later if necessary.

becker33 · 2019-01-05T02:56:53Z

@scheibelp comments addressed.

…xed existing tests

…r normal projection

…in projections.yaml

…iew.root to view._root

becker33 · 2019-01-08T23:56:11Z

@scheibelp ping

scheibelp

I have a few minor requests related to clarity/documentation.

I will continue reviewing this tomorrow morning.

lib/spack/docs/workflows.rst

lib/spack/spack/build_systems/python.py

lib/spack/spack/cmd/view.py

becker33 · 2019-01-09T19:08:34Z

@scheibelp comments addressed

scheibelp

I have a few requests/questions about the tests.

lib/spack/spack/test/test_activations.py

scheibelp · 2019-01-09T22:21:14Z

lib/spack/spack/test/test_activations.py

+    old_path = spack.repo.path
+    repo_dirs = [spack.paths.packages_path, spack.paths.mock_packages_path]
+    path = RepoPath(*repo_dirs)
+    sys.meta_path.append(path)


Could this use repo.swap. Although there is minimal duplication, it's useful to minimize the number of unique manipulations of sys.meta_path.

I'm a little bit worried that repo.swap doesn't restore sys.meta_path properly, but that's a separate scope of work than this PR and this PR should use the logic there. We can fix it separately if there's an issue.

If the path added by repo.swap is already in sys.meta_path, won't repo.swap accidentally change it from its current location to the end of sys.meta_path (because list.remove removes the first instance in the list, not the last)? If so, we can fix this by either caching sys.meta_path the same way we cache repo.path or by reversing the list, removing the item, and reversing it again.

If the path added by repo.swap is already in sys.meta_path, won't repo.swap accidentally change it from its current location to the end of sys.meta_path (because list.remove removes the first instance in the list, not the last)?

I think you're right. Generally I think this just hasn't happened yet - the same Repo/RepoPath is not added multiple times. I think it's worth sorting out in a separate PR. Generally I assume that it should be disallowed to add it multiple times (raise an exception).

Testing could be a reason to add the same one multiple times, but if we need that there's other ways to ensure we don't mess up the ordering.

lib/spack/spack/test/test_activations.py

var/spack/repos/builtin.mock/packages/perl/package.py

…on files

scheibelp · 2019-01-10T01:39:45Z

Thanks!

tgamblin requested review from scheibelp and tgamblin October 31, 2018 00:32

becker33 force-pushed the features/projections branch 4 times, most recently from 210de14 to 030f312 Compare October 31, 2018 18:48

tgamblin closed this Nov 21, 2018

tgamblin reopened this Nov 21, 2018

becker33 force-pushed the features/projections branch from 0a4f958 to c612d4f Compare December 13, 2018 01:41

tgamblin mentioned this pull request Dec 20, 2018

Maintain a view for an environment #10017

Merged

scheibelp requested changes Jan 4, 2019

View reviewed changes

becker33 added 18 commits January 7, 2019 09:44

Initial implementation combinatorial view projections

691c760

Fixed status and cleaned up interface. Fixed relaxed_disambiguate. Fi…

5e7ff46

…xed existing tests

Add new mock packages for old tests to continue working

5be4390

Make extension metadata and status reflect where they linked not thei…

203867a

…r normal projection

Make extensions understand projections, make views store projections …

0ca0a90

…in projections.yaml

Add testing for combinatorial views

5f30c2c

use syaml_dict instead of OrderedDict for python2.6 compatibility

fc1ecfb

Allow spec format to include dependency information

0546285

Write empty projection into file if necessary

3edbb75

Document combinatorial views

0571305

flake

f6d1075

Update old license headers

69fd99a

Cleanup print statements

d337f12

add schema validation for projections

6e77039

Add schema file

e7c6cef

flake

8687e6b

Addressed review, make 'all' location-agnostic in projections.yaml

08d7928

rename disambiguation function and die if specified file doesn't exist

2487f57

post-rebase cleanup

978f2f4

becker33 force-pushed the features/projections branch from a34132f to 978f2f4 Compare January 7, 2019 18:03

becker33 added 3 commits January 8, 2019 13:20

Do not use root as interface to a view

045e5bd

Remove unnecessary duplicate view from extension adding logic, move v…

bf8352e

…iew.root to view._root

flake

b42ef9e

scheibelp requested changes Jan 9, 2019

View reviewed changes

lib/spack/docs/workflows.rst Outdated Show resolved Hide resolved

lib/spack/spack/build_systems/python.py Outdated Show resolved Hide resolved

lib/spack/spack/cmd/view.py Outdated Show resolved Hide resolved

lib/spack/spack/cmd/view.py Outdated Show resolved Hide resolved

becker33 added 2 commits January 9, 2019 11:06

Update variable names for global view consistency

6f3286b

workflows documentation: explain single-prefix views first

b70c698

Create reusable validator for extant files for argparsse

7ad8e1f

becker33 force-pushed the features/projections branch from 2236e98 to 7ad8e1f Compare January 9, 2019 19:56

update view command description

eff30d9

becker33 force-pushed the features/projections branch from 8aebf6a to eff30d9 Compare January 9, 2019 21:38

scheibelp requested changes Jan 9, 2019

View reviewed changes

becker33 added 4 commits January 9, 2019 15:10

refactor builtin_and_mock_packages fixture to use repo.swap

6218777

Add cmd/view test that argparse properly rejects nonexistent projecti…

35ba622

…on files

cleanup

cf158c3

updated comment in builtin_and_mock_packages fixture

638cf68

scheibelp approved these changes Jan 10, 2019

View reviewed changes

scheibelp merged commit 450b0e3 into develop Jan 10, 2019

tgamblin deleted the features/projections branch January 12, 2019 09:21

becker33 mentioned this pull request Apr 24, 2019

Simplified directory layout trees with symlinks #2168

Closed

Conversation

becker33 commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

becker33 commented Jan 5, 2019

Uh oh!

becker33 commented Jan 8, 2019

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

becker33 commented Jan 9, 2019

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

scheibelp commented Jan 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

becker33 commented Oct 30, 2018 •

edited

Loading