Added test for Issue #682 and fixed. by jpmccu · Pull Request #718 · RDFLib/rdflib (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation9 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
@joernhees, can I get some help from someone who knows python 3 better than I on these failures?
The current error (in Py3) seems to expect IsomorphicGraph to be hashable:
...
File "/home/travis/build/RDFLib/rdflib/rdflib/plugins/memory.py", line 254, in add
self.__all_contexts.add(context)
TypeError: unhashable type: 'IsomorphicGraph'
:-/ sad, i thought maybe just removing the py2 print statements already fixes it...
aaaahaaa:
https://docs.python.org/3/reference/datamodel.html#object.__hash__
A class that overrides __eq__() and does not define __hash__() will have its __hash__() implicitly set to None. When the __hash__() method of a class is None, instances of the class will raise an appropriate TypeError when a program attempts to retrieve their hash value, and will also be correctly identified as unhashable when checking isinstance(obj, collections.Hashable).
in py3 overriding eq implicitly resets hash
@jimmccusker tests pass, but i'm not entirely sure f72b51a is the way to go... the store uses the hash as context (quads), so changing it as new triples are added is bad bad bad... however, hashing the full IsomorphicGraph should probably have something to do with its content?!? luckily we also have an IsomorphicGraph.internal_hash 🙄
Right, that's the thing. __hash__ is supposed to be invariant. What does set do? It's hashable, but variant. Since RDF graphs are supposed to be sets of triples (or quads), maybe we can look there for guidance.
Python sets are NOT hashable, precisely because they are mutable. That's why there is "FrozenSet", this thing has to copy all the triples into a new object that is unchangable. OR make a "FrozenGraph" subclass, that does not allow writes, and hopes the underlying store is not changed during the operation of checking isomorphism.
Then that argues that Graphs shouldn't be hashable either. For some reason, there seems to be a use case in Python 3 where graphs are expected to be hashable, but not in Python 2.7.
hmm, not sure we're not getting hung up here on the alternative fact that IOMemory internally hashes the graph's identifier to transform triples into quads... i'm leaning toward hitting that the merge button and be done with this for now, but maybe we should add a cleanup issue to remove the __hash__ from all mutable things and make IOMemory explicitly hash the identifier?
Labels
Something isn't working