RF: revert to default cmp=True for attr.s. Rely on frozen to get hash generated

[yarikoptic]

It is not clear if we really had to disable cmp which hinders simple comparison of our objects.
Having some tests still failing locally (yet to troubleshoot), submitting to CI

[codecov[bot]]

Codecov Report

Merging #430 into master will decrease coverage by 0.12%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
- Coverage   89.95%   89.82%   -0.13%     
==========================================
  Files         148      148              
  Lines       11742    12206     +464     
==========================================
+ Hits        10562    10964     +402     
- Misses       1180     1242      +62

Impacted Files	Coverage Δ
reproman/distributions/docker.py	89.74% <100%> (ø)	:arrow_up:
reproman/distributions/singularity.py	94.66% <100%> (ø)	:arrow_up:
reproman/distributions/redhat.py	94.51% <100%> (ø)	:arrow_up:
reproman/distributions/debian.py	95.3% <100%> (ø)	:arrow_up:
reproman/resource/tests/test_ssh.py	78.12% <0%> (-21.88%)	:arrow_down:
reproman/interface/tests/test_execute.py	87.59% <0%> (-12.41%)	:arrow_down:
reproman/resource/ssh.py	79.41% <0%> (-7.48%)	:arrow_down:
reproman/interface/execute.py	93.53% <0%> (-2.92%)	:arrow_down:
reproman/interface/create.py	94.11% <0%> (-0.89%)	:arrow_down:
reproman/resource/tests/test_session.py	98.98% <0%> (-0.57%)	:arrow_down:
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c30a0ae...fae7a7e. Read the comment docs.

[yarikoptic]

So according to the tests at least it is all kosher... Will check more locally and merge if I don't run into side effects

[kyleam]

It is not clear if we really had to disable cmp which hinders simple comparison of our objects.

FWIW I also wasn't able to find any rationale for this after poking around a bit in the commit history, issues, and comments.

Somewhat related attrs best practices question: Should we avoid explicitly setting hash? The documentation suggests it should be left at its default value for both attr.s and attr.ib.

[yarikoptic]

I guess I will need to check commits around that point in time to recall to which dict we were adding those instances... May be it is no longer needed indeed, at least until https://github.com/ReproNim/reproman/pull/419 requires it

[yarikoptic]

I have looked around

• it came from https://github.com/ReproNim/reproman/pull/121 by @chaselgrove with motivation to use DEBPackage immutable and used in sets, feel like for a diff functionality.

Should we avoid explicitly setting hash?

I guess it depends on either we still depend on it as an ability to place those objects into some set or dict, to match their "identity". now there is also the _diff_cmp_fields attributes set to pretty much reflect the same semantic -- which fields are to be used to identify any SpecObject. The other fields should not "participate" in identification. In identify_packages_from_files we seems to not rely/use that (if used before) and instead rely on _get_packagefields_for_files which returns dict of the fields and then we explicitly convert to a tuple (for all fields) to add to a dict. I am not sure if hashing is still used somewhere for diff or we already moved to use explicit list defined in _diff_cmp_fields (question would be to @chaselgrove)?

[kyleam]

Should we avoid explicitly setting hash?

I guess it depends on either we still depend on it as an ability to place those objects into some set or dict, to match their "identity".

The point isn't that hashing objects should be avoided; it's that hash should be determined by frozen and cmp rather than explicitly set. Quoting from the top of the page I linked to:

Warning

The overarching theme is to never set the @attr.s(hash=X) parameter yourself. Leave it at None which means that attrs will do the right thing for you, depending on the other parameters:

• If you want to make objects hashable by value: use @attr.s(frozen=True).
• If you want hashing and comparison by object identity: use @attr.s(cmp=False)

Setting hash yourself can have unexpected consequences so we recommend to tinker with it only if you know exactly what you’re doing.

To use our DEBPackage as an example, following this advice for attr.s (and leaving the attr.ib's alone for now) would translate to

diff --git a/reproman/distributions/debian.py b/reproman/distributions/debian.py
index b1de5c102..205b5e4d5 100644
--- a/reproman/distributions/debian.py
+++ b/reproman/distributions/debian.py
@@ -69,7 +69,7 @@ class APTSource(SpecObject):
 _register_with_representer(APTSource)

-@attr.s(slots=True, frozen=True, hash=True)
+@attr.s(slots=True, frozen=True)
 class DEBPackage(Package):
     """Debian package information"""
     name = attrib(default=attr.NOTHING)

The object would still be hashable:

% python -c 'from reproman.distributions.debian import DEBPackage; hash(DEBPackage(name="blah"))'

[yarikoptic]

ok, pushed fae7a7edf0013248a55fa585e244ee6b57448103 which removed explicit hash=True from attr.s

[yarikoptic]

So let's consider that no actual functionality is broken and merge?

[kyleam]

@yarikoptic:

So let's consider that no actual functionality is broken and merge?

Given that no objections have been raised, sounds fine to me.