database/spec: Keep track of dependents using object identity instead#16447
database/spec: Keep track of dependents using object identity instead#16447aweits wants to merge 6 commits intospack:developfrom
Conversation
|
Once the semester is over (before or after this PR gets merged) I'll check to see if this fixes #3690. |
|
Just tested this out. I still have an error when uninstalling a lot of packages. |
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
I'm not really sure id is the right granularity for this reason:
Spack version 0.14.2
Python 3.6.9, Linux x86_64
>>> import spack.spec
>>> hdf5 = spack.spec.Spec('hdf5')
>>> hdf5.concretize()
>>> zlib = hdf5['zlib']
>>> zlib_copy = zlib.copy()
>>> id(zlib)
140656426554088
>>> id (zlib_copy)
140656426527712
>>> zlib.dag_hash()
'os3djb57dqdzhaqsl2n7iefz3gwombyi'
>>> zlib_copy.dag_hash()
'os3djb57dqdzhaqsl2n7iefz3gwombyi'In my opinion we don't care whether it's the same Python object, but if it represents the same build.
Another thing to consider is how we want to use _dependents. Using Spec.dag_hash instead of id is fine way to solve the issue above and get a consistent data structure. Another way to solve #11983 would be to continue indexing by name but use a list of specs as a value. I guess what is the best solution depends on how do we envision using _dependents. Thoughts?
Long-term, I do concur, I have another branch where I'm working on this - Spec needs to be much more defensive with respect to how things that are a part of the hash are changed/altered (i.e. simply using dag_hash() inside of _add_dependencies is fraught with peril). I felt I should at least put this forward as an potential interim fix. |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
Currently I don't see any issue with using id, but I want to consider the possible issues a bit more before approving.
I’m wondering if we could replace the dictionary with a list, or some custom object which mostly behaves like a list but would offer the ability to access elements based on object-id or spec equality (or for concrete specs, the DAG hash). This would also help track down possible reference issues e.g. right now _replace_with still refers to the keys as "name" (which doesn't cause a bug because they aren't used, but may confuse future readers).
| @pytest.mark.regression('11983') | ||
| def test_multiple_dependents(database): | ||
| specs = spack.store.db.query('dyninst') | ||
| assert len(specs) == 1 |
There was a problem hiding this comment.
This assertion feels somewhat fragile: we are depending on the fake installation to maintain a single dyninst install going forward. That being said, if there were multiple that would also cause a problem for this test since the callpath dependents might change. Ideally then this could start with a mock database and repo, and install multiple versions of some simple package X depending on package Y
There was a problem hiding this comment.
I can certainly change that.
|
Closing this one in favor of #21683 |
4 participants
Fixes #11983