A series of changes I've been using to improve external packages by krafczyk · Pull Request #65 · epfl-scitas/spack

krafczyk · 2017-04-04T17:01:21Z

Some highlights:

I change spec.external to be a simple boolean and add spec.external_path
I add some more debug messages
There's no need to find the external module path during every loop of concreization..
A loop can happen during concretization where an external spec which is not concrete is used for a package. This package is then concretized furthur. That spec is then tested to see whether an external spec matches it, and the old unconcretized spec replaces the one we just concretized thus starting an infinite loop.

it's not necessary.. The paths are retrieved at the end of concretizaion

No reason not to test if the root of the DAG is external when external packages are now first class citizens!

alalazo

I need to do a test-drive with it, but at a first glance it LGTM. I have only one concern about self.external expressed below.

If @tgamblin approves the changes here I'll merge them in the branch after we finish iterating over them.

alalazo · 2017-04-05T13:01:02Z

lib/spack/spack/package.py

+            tty.msg(message.format(s=self, path=self.spec.external_path))
+        else:
+            message = '{s.name}@{s.version} : externally installed in {path}'
+            tty.msg(message.format(s=self, path=self.spec.external_path))


This change is definitely a 👍

alalazo · 2017-04-05T13:05:56Z

lib/spack/spack/spec.py

-        self.external = kwargs.get('external', None)
+        self.external = kwargs.get('external', False)
+        self.external_path = kwargs.get('external_path', None)
        self.external_module = kwargs.get('external_module', None)


I see your point in having a bool as self.external, but I am worried that the way it is coded now won't enforce consistency among the three attributes above. What do you think of having a property instead:

@property def external(self): return bool(self.external_path) or bool(self.external_module)

?

Yes that's better. I'll push a commit in a little while.

krafczyk · 2017-04-05T16:27:32Z

@alazo, I've added an external property. Tell me what you think.

Canonicalize external package paths so that users may specify their locations relative to spack's directory.

alalazo

There are unit tests that fail (9 on my machine). Some of the errors seem trivial (the test was not updated) other probably need a deeper revision of the logic here.

alalazo · 2017-04-06T20:01:39Z

lib/spack/spack/spec.py

        spec.architecture = None
        spec.compiler = None
-        spec.external = None
+        spec.external = False


@krafczyk I think the line above is a leftover.

alalazo · 2017-04-06T20:09:42Z

For convenience, here's the result of the tests on my Ubuntu 14:

$ spack test
=============================================================================================== test session starts ===============================================================================================
platform linux2 -- Python 2.7.6, pytest-3.0.5, py-1.4.32, pluggy-0.4.0
rootdir: /home/mculpo/PycharmProjects/spack, inifile: pytest.ini
plugins: xdist-1.15.0, cov-2.4.0
collected 531 items 

lib/spack/spack/test/architecture.py .......
lib/spack/spack/test/build_system_guess.py .........
lib/spack/spack/test/build_systems.py .
lib/spack/spack/test/cc.py ................
lib/spack/spack/test/compilers.py ..
lib/spack/spack/test/concretize.py ...................................F..F......
lib/spack/spack/test/concretize_preferences.py .......F
lib/spack/spack/test/config.py ............
lib/spack/spack/test/database.py ....FFF..FF....F
lib/spack/spack/test/directory_layout.py ...
lib/spack/spack/test/environment.py .........
lib/spack/spack/test/file_cache.py ..
lib/spack/spack/test/git_fetch.py ....
lib/spack/spack/test/graph.py ....
lib/spack/spack/test/hg_fetch.py ..
lib/spack/spack/test/install.py ...
lib/spack/spack/test/library_list.py ......
lib/spack/spack/test/link_tree.py ....
lib/spack/spack/test/lock.py ....................................
lib/spack/spack/test/make_executable.py ......
lib/spack/spack/test/mirror.py .....
lib/spack/spack/test/modules.py ................
lib/spack/spack/test/multimethod.py ..........
lib/spack/spack/test/namespace_trie.py .....
lib/spack/spack/test/optional_deps.py ..................
lib/spack/spack/test/package_sanity.py ....
lib/spack/spack/test/packages.py ............
lib/spack/spack/test/pattern.py ...
lib/spack/spack/test/provider_index.py .....
lib/spack/spack/test/python_version.py ..
lib/spack/spack/test/sbang.py ..
lib/spack/spack/test/spack_yaml.py ....
lib/spack/spack/test/spec_dag.py ......................................
lib/spack/spack/test/spec_semantics.py ...................................
lib/spack/spack/test/spec_syntax.py ..............................
lib/spack/spack/test/spec_yaml.py ........
lib/spack/spack/test/stage.py ................
lib/spack/spack/test/svn_fetch.py ..
lib/spack/spack/test/url_extrapolate.py ........
lib/spack/spack/test/url_parse.py ...........................................................
lib/spack/spack/test/url_substitution.py ..
lib/spack/spack/test/versions.py .................................
lib/spack/spack/test/cmd/find.py .
lib/spack/spack/test/cmd/install.py ..
lib/spack/spack/test/cmd/module.py ......
lib/spack/spack/test/cmd/python.py .
lib/spack/spack/test/cmd/test_compiler_cmd.py ..
lib/spack/spack/test/cmd/uninstall.py .
lib/spack/spack/test/cmd/url.py ...x..
============================================================================================= short test summary info =============================================================================================
FAIL lib/spack/spack/test/concretize.py::TestConcretize::()::test_external_package
FAIL lib/spack/spack/test/concretize.py::TestConcretize::()::test_external_and_virtual
FAIL lib/spack/spack/test/concretize_preferences.py::TestConcretizePreferences::()::test_external_mpi
FAIL lib/spack/spack/test/database.py::test_020_db_sanity
FAIL lib/spack/spack/test/database.py::test_025_reindex
FAIL lib/spack/spack/test/database.py::test_030_db_sanity_from_another_process
FAIL lib/spack/spack/test/database.py::test_060_remove_and_add_root_package
FAIL lib/spack/spack/test/database.py::test_070_remove_and_add_dependency_package
FAIL lib/spack/spack/test/database.py::test_external_entries_in_db
XFAIL lib/spack/spack/test/cmd/url.py::test_url_parse_xfail

============================================================================================ slowest 20 test durations ============================================================================================
58.47s call     lib/spack/spack/test/modules.py::TestTcl::test_autoload
33.27s call     lib/spack/spack/test/modules.py::TestTcl::test_prerequisites
32.71s call     lib/spack/spack/test/modules.py::TestLmod::test_autoload
23.11s call     lib/spack/spack/test/modules.py::TestTcl::test_blacklist
13.32s call     lib/spack/spack/test/modules.py::TestDotkit::test_dotkit
10.80s call     lib/spack/spack/test/modules.py::TestLmod::test_blacklist
8.38s call     lib/spack/spack/test/modules.py::TestLmod::test_alter_environment
7.40s call     lib/spack/spack/test/modules.py::TestTcl::test_alter_environment
7.23s call     lib/spack/spack/test/modules.py::TestTcl::test_conflicts
7.09s call     lib/spack/spack/test/modules.py::TestTcl::test_setup_environment
7.01s setup    lib/spack/spack/test/cmd/uninstall.py::test_uninstall
6.49s setup    lib/spack/spack/test/cmd/module.py::test_exit_with_failure[failure_args0]
6.43s setup    lib/spack/spack/test/spec_syntax.py::TestSpecSyntax::test_spec_by_hash
5.76s call     lib/spack/spack/test/python_version.py::PythonVersionTest::test_core_module_compatibility
5.01s setup    lib/spack/spack/test/database.py::test_default_queries
4.74s teardown lib/spack/spack/test/database.py::test_030_db_sanity_from_another_process
4.22s call     lib/spack/spack/test/cmd/url.py::test_url_list
3.48s call     lib/spack/spack/test/modules.py::TestLmod::test_simple_case
3.41s call     lib/spack/spack/test/modules.py::TestTcl::test_simple_case
3.35s call     lib/spack/spack/test/python_version.py::PythonVersionTest::test_package_module_compatibility
==================================================================================================== FAILURES =====================================================================================================
...
================================================================================ 9 failed, 521 passed, 1 xfailed in 293.25 seconds ================================================================================

krafczyk · 2017-04-06T20:22:40Z

I'm able re reproduce these test failures, so I'm going about fixing them.

krafczyk · 2017-04-06T21:02:43Z

@alazo I've fixed the test failures. There was nothing extensive to fix, the tests just needed to be updated.

alalazo · 2017-04-07T09:25:16Z

@krafczyk The tests now pass, thanks. I'll have time to try the branch on a few use cases over the week-end and will get back to you.

tgamblin · 2017-04-20T07:50:06Z

@krafczyk: thanks! these are useful refactors

* Improve implementation of external packages * Don't find external module path at each step of concretization it's not necessary.. The paths are retrieved at the end of concretizaion * Don't find replacements for external packages. * Test root of the DAG if external No reason not to test if the root of the DAG is external when external packages are now first class citizens! * Clean up statements in package for external modules * Correct serious flaw with external package code I made earlier * Create external property for Spec. * Remove some stray externals * Allow users to specify external package paths relative to spack Canonicalize external package paths so that users may specify their locations relative to spack's directory. * Remove leftover external= statement * Update tests to use new external_path and external properly.

krafczyk added 6 commits April 4, 2017 10:40

Improve implementation of external packages

02868c8

Don't find external module path at each step of concretization

f9ea56c

it's not necessary.. The paths are retrieved at the end of concretizaion

Don't find replacements for external packages.

fc082cc

Test root of the DAG if external

5b45698

No reason not to test if the root of the DAG is external when external packages are now first class citizens!

Clean up statements in package for external modules

ba96d41

Correct serious flaw with external package code I made earlier

14a00e2

krafczyk mentioned this pull request Apr 4, 2017

feature : register external packages in the DB spack/spack#1167

Merged

3 tasks

alalazo reviewed Apr 5, 2017

View reviewed changes

krafczyk added 2 commits April 5, 2017 11:20

Create external property for Spec.

447da18

Remove some stray externals

9014567

Allow users to specify external package paths relative to spack

14d92e7

Canonicalize external package paths so that users may specify their locations relative to spack's directory.

alalazo requested changes Apr 6, 2017

View reviewed changes

krafczyk added 2 commits April 6, 2017 15:59

Remove leftover external= statement

4fd41eb

Update tests to use new external_path and external properly.

6c81201

alalazo approved these changes Apr 7, 2017

View reviewed changes

alalazo merged commit 24cb9aa into epfl-scitas:features/external_packages_are_first_class_citizens Apr 20, 2017

rmsds pushed a commit that referenced this pull request Jul 30, 2018

deployement of i7z (#65)

93ef740

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A series of changes I've been using to improve external packages#65

A series of changes I've been using to improve external packages#65
alalazo merged 11 commits intoepfl-scitas:features/external_packages_are_first_class_citizensfrom
krafczyk:fix/external_packages_mods

krafczyk commented Apr 4, 2017 •

edited

Loading

Uh oh!

alalazo left a comment

Uh oh!

alalazo Apr 5, 2017

Uh oh!

alalazo Apr 5, 2017

Uh oh!

krafczyk Apr 5, 2017

Uh oh!

krafczyk commented Apr 5, 2017

Uh oh!

alalazo left a comment

Uh oh!

alalazo Apr 6, 2017

Uh oh!

alalazo commented Apr 6, 2017

Uh oh!

krafczyk commented Apr 6, 2017

Uh oh!

krafczyk commented Apr 6, 2017

Uh oh!

alalazo commented Apr 7, 2017

Uh oh!

tgamblin commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

krafczyk commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

alalazo Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

krafczyk Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

krafczyk commented Apr 5, 2017

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

alalazo commented Apr 6, 2017

Uh oh!

krafczyk commented Apr 6, 2017

Uh oh!

krafczyk commented Apr 6, 2017

Uh oh!

alalazo commented Apr 7, 2017

Uh oh!

tgamblin commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krafczyk commented Apr 4, 2017 •

edited

Loading