Use spack.store.use_store consistently in unit tests #21656

alalazo · 2021-02-12T23:37:00Z

#21442 missed a few places in unit tests where the store was monkeypatched within tests. This may cause spurious failures, since monkeypatching was not done consistently over all the global variables in spack.store.

Other modifications:

spack.store.use_store has been updated to also swap spack.store.db, spack.store.layout, spack.store.root and spack.store.unpadded_root
spack.store.store is serialized in subprocess_context.py on Python > 3.8 + MacOS

alalazo · 2021-02-13T17:24:26Z

@tgamblin This is a follow-up on #21442 In that PR I didn't notice we have tests that monkey patch spack.store.*. This is fixed here. Getting tests working correctly on MacOS + Python 3.8+ required serializing also the store, so it would be great if @scheibelp could take a look at least at the last commit in the PR.

tgamblin

I have one request for comment, and I think @scheibelp should review this.

tgamblin · 2021-02-16T17:31:29Z

lib/spack/spack/binary_distribution.py

@@ -1119,7 +1121,6 @@ def relocate_package(spec, allow_root):
    prefix_to_prefix_bin = OrderedDict({})

    if old_sbang_install_path:
-        import spack.hooks.sbang as sbang


There was some kind of circular import issue on certain platforms that led to this being nested here. Can you look at the blame for these nested sbang imports and see if you can figure out whether it's really been resolved? I think @eugeneswalker and @scheibelp should comment.

The imports are still nested, but they are done at the top of the function. This is because imports act like a local assignment to the spack variable and using any spack.* module or function before the nested import will fail with weird errors. Checking git blame right now.

Only reference found for these imports #20768 (comment)

tgamblin · 2021-02-16T17:31:40Z

lib/spack/spack/binary_distribution.py

@@ -1133,7 +1134,6 @@ def relocate_package(spec, allow_root):
    # sbang was a bash script, and it lived in the spack prefix. It is
    # now a POSIX script that lives in the install prefix. Old packages
    # will have the old sbang location in their shebangs.
-    import spack.hooks.sbang as sbang


see above comment on circular imports.

scheibelp

I have a number of small requests and a larger concern about the consistency of store state when running upstream tests in test/install.py:

This appeared to set store.db and store without addressing the TODO above, so I'm trying to figure out why tests like test_installed_upstream are still passing

I don't have a ready solution for that but we can brainstorm how to deal with it if you'd like.

lib/spack/spack/store.py

lib/spack/spack/test/directory_layout.py

lib/spack/spack/test/conftest.py

scheibelp · 2021-02-16T19:37:49Z

lib/spack/spack/test/conftest.py

-        _install('mpileaks ^mpich2')
-        _install('mpileaks ^zmpi')
-        _install('externaltest')
+    _install('mpileaks ^mpich')


Why is the mock_db.write_transaction call removed here?

As far as I reconstructed, this context manager was used since #2502. Initially it was a way to speed-up the fixture by grabbing a write lock early instead of repeatedly take and release the lock for each installation (like Spack would do in its normal operations). This made sense since database or mutable_database were installing and uninstalling specs at least once per python file. Since some time though the installations are done just once per session, moved to a cache directory and copied wherever they are needed without the additional burden of installing and uninstalling them again. All this is just to say that we will not lose anything in terms of test performance by removing the context manager.

Instead, the necessity to remove it in this PR comes from the additional serialization I do of attributes related to the store. When we fork a process and we _serialize we create an equivalent, but different, store object in the forked process. If a write lock for the same DB is held by the parent process that will prevent the child to proceed.

lib/spack/spack/test/config_values.py

lib/spack/spack/subprocess_context.py

spack#21442 missed a few places in unit tests where the store was monkeypatched within tests. This may cause spurious failures, since monkeypatching was not done consistently over all the global variables in spack.store The spack.store.use_store context manager has been updated to also swap spack.db, spack.layout, spack.root and spack.unpadded_root consistently

- Set store attributes on different lines - Add a comment explaining the call to _store()

Before this commit there were local fixtures replicating the same behavior of creating a temporary local store.

- Add a comment explaining the call to _store() - Use temporary_store in install_mockery

scheibelp

I think this refactor is looking excellent. Especially if we can remove all calls which monkeypatch either the store or db, i.e. all calls like:

monkeypatch.setattr(spack.store, 'db'
monkeypatch.setattr(spack.store, 'store'

I think all such calls have been handled at this point. Calls like

monkeypatch.setattr(spack.store.layout, 'build_packages_path', bpp_path)

should be fine.

I have a couple additional requests related to documentation

scheibelp · 2021-02-17T18:21:25Z

lib/spack/spack/test/conftest.py

+    temporary_store_path = tmpdir.join('opt')
+    with spack.store.use_store(str(temporary_store_path)) as s:
+        yield s
+        temporary_store_path.remove()


Is it necessary to remove this? I'd assume the tmpdir fixture would handle that for you. If it needs to happen, I think it should happen outside of the context manager.

There's one test in bindist.py that fails without this line, so I moved it outside of the context manager. We can investigate in a later PR why the test is failing. For what is worth I was under the impression that the tmpdir had to be cleanup immediately after a succesful test, but it seems a few of them are kept around at any time, see e.g. here

lib/spack/spack/test/install.py

- Moved tmpdir path removal outside of context manager - Added docstring for install_upstream

scheibelp

I have a final request regarding how the Store object is serialized. I think this is good to go after that. Do you agree with the following commit message:

Testing: use spack.store.use_store everywhere (#21656)

Keep spack.store.store and spack.store.db consistent in unit tests

* Remove calls to monkeypatch for spack.store.store and spack.store.db:
  tests that used these called one or the other, which lead to
  inconsistencies (the tests passed regardless but were fragile as a
  result)
* Fixtures making use of monkeypatch with mock_store now use the
  updated use_store function, which sets store.store and store.db
  consistently
* subprocess_context.TestState now transfers the serializes and
  restores spack.store.store (without the monkeypatch changes this
  would have created inconsistencies)

lib/spack/spack/store.py

lib/spack/spack/subprocess_context.py

alalazo · 2021-02-18T19:53:37Z

The commit message is great, I completely agree with that.

Request for comment has been addressed

scheibelp · 2021-02-18T21:22:56Z

Thanks!

alalazo added tests General test capability(ies) ci Issues related to Continuous Integration labels Feb 12, 2021

alalazo marked this pull request as ready for review February 13, 2021 17:20

alalazo requested review from scheibelp, becker33, tgamblin and tldahlgren February 13, 2021 17:20

alalazo added this to In progress in Spack 0.17.0 Release via automation Feb 14, 2021

This was referenced Feb 15, 2021

Bootstrap clingo from sources #21446

Merged

Allow for multiple dependencies/dependents from the same package #21683

Closed

tgamblin previously requested changes Feb 16, 2021

View reviewed changes

Spack 0.17.0 Release automation moved this from In progress to Review in progress Feb 16, 2021

scheibelp requested changes Feb 16, 2021

View reviewed changes

alalazo added 5 commits February 17, 2021 10:06

fixup

0bab509

Fix serialization of store for MacOS and Python > 3.8

488b355

Addressed minor review comments

82a1066

- Set store attributes on different lines - Add a comment explaining the call to _store()

Use a unique fixture to create temporary stores

cc64c5f

Before this commit there were local fixtures replicating the same behavior of creating a temporary local store.

alalazo force-pushed the fixes/spack_store_use_store branch from 482e0e0 to cc64c5f Compare February 17, 2021 12:06

alalazo added 2 commits February 17, 2021 15:32

Avoid monkeypatching spack.store.db

a644293

Addressed minor review comments, take 2

a21e1e6

- Add a comment explaining the call to _store() - Use temporary_store in install_mockery

scheibelp requested changes Feb 17, 2021

View reviewed changes

Addressed review comments

f3ceee6

- Moved tmpdir path removal outside of context manager - Added docstring for install_upstream

scheibelp requested changes Feb 18, 2021

View reviewed changes

lib/spack/spack/store.py Show resolved Hide resolved

lib/spack/spack/subprocess_context.py Outdated Show resolved Hide resolved

Store: added serialize and deserialize method

d3abc7a

scheibelp approved these changes Feb 18, 2021

View reviewed changes

scheibelp merged commit f2e3edf into spack:develop Feb 18, 2021

Spack 0.17.0 Release automation moved this from Review in progress to Done Feb 18, 2021

alalazo deleted the fixes/spack_store_use_store branch February 18, 2021 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use spack.store.use_store consistently in unit tests #21656

Use spack.store.use_store consistently in unit tests #21656

alalazo commented Feb 12, 2021 •

edited

alalazo commented Feb 13, 2021

tgamblin left a comment

tgamblin Feb 16, 2021

alalazo Feb 16, 2021

alalazo Feb 16, 2021

tgamblin Feb 16, 2021

scheibelp left a comment

scheibelp Feb 16, 2021

alalazo Feb 17, 2021

scheibelp left a comment

scheibelp Feb 17, 2021

alalazo Feb 18, 2021

scheibelp left a comment

alalazo commented Feb 18, 2021

scheibelp commented Feb 18, 2021

Use spack.store.use_store consistently in unit tests #21656

Use spack.store.use_store consistently in unit tests #21656

Conversation

alalazo commented Feb 12, 2021 • edited

alalazo commented Feb 13, 2021

tgamblin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp left a comment

Choose a reason for hiding this comment

alalazo commented Feb 18, 2021

scheibelp commented Feb 18, 2021

alalazo commented Feb 12, 2021 •

edited