classification
Title: Path-based loaders lack a meaningful __eq__() implementation.
Type: behavior Stage: needs patch
Components: Versions: Python 3.4
process
Status: closed Resolution:
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: brett.cannon, eric.snow, larry, ncoghlan, python-dev
Priority: high Keywords: patch

Created on 2013-12-08 05:08 by eric.snow, last changed 2014-01-05 07:12 by eric.snow. This issue is now closed.

Files
File name Uploaded Description Edit
issue19927-loader-eq.diff eric.snow, 2013-12-10 03:16 review
Messages (28)
msg205516 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-08 05:13
(my browser farted the half finished report into existence :P )

The __eq__() implementation of the path-based loaders in importlib is just the stock one that compares object identity.  So two that are effectively the same compare unequal.  This has a material impact on ModuleSpec.  ModuleSpec.__eq__() does a comparision of its various attributes, one of them being the loader.  Thus most specs will compare unequal even though they are effectively equal.

I recommend that we provide a better implementation for SourceFileLoader and friends.

Larry: would such a feature addition be okay?
msg205518 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-08 06:16
Here's a patch.
msg205521 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-08 06:31
There can be some interesting backwards compatibility consequences when adding an __eq__ implementation to a class that was previously using the default ID based __eq__:

- it becomes unhashable (unless you also add a suitable __hash__ definition)

- subclasses with additional significant state may start comparing equal, even though their additional state is not taken into account by the new __eq__ function.

For the latter problem, you can alleviate it by comparing the instance dictionaries rather than specific attributes:

>>> class Example:
...     def __eq__(self, other):
...         return self.__class__ == other.__class__ and self.__dict__ == other.__dict__
... 
>>> a = Example()
>>> b = Example()
>>> a == b
True
>>> a.foo = 1
>>> a == b
False
>>> b.foo = 1
>>> a == b
True

(technically this can still change subclass behaviour if they're messing about with slots, but there *is* such a thing as being *too* paranoid about backwards compatibility)

The hashability problem is easy enough to handle just by mixing together the hashes of the attributes of most interest:

    def __hash__(self):
        return hash(self.name) ^ hash(self.path)
msg205553 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-12-08 12:24
Brett, could you weigh in please?
msg205577 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-12-08 16:15
I'm fine with the suggestions Nick made. While loaders are not technically immutable (and thus technically probably shouldn't define __hash__), they have not been defined to be mutable and mucked with anyway, so I have no issue if someone breaks the hash of a loader by changing an attribute post-hash.
msg205768 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-10 03:16
Good point, Nick.  Here's a patch that follows your recommendation on both points.
msg206669 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-20 08:45
Unless there are objections, I'll commit this in the next day or two.
msg206767 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-12-21 18:31
That's not how this works, Eric.  I have to give you permission to add a new feature, which I remind you I have yet to do.
msg206771 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-21 19:41
My bad, Larry.  I guess I was reading between the lines too much. :)
msg206802 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-22 03:03
That reminds me: I ended up working around this in the runpy tests by only
checking the loader type was correct in the module specs. With an improved
definition of equality for loaders, the runpy tests could be both
simplified *and* made more rigorous at the same time (by simply comparing
specs for equality, the same as the other module level attributes).
msg206803 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-22 03:10
Yeah, it was while writing tests that I ran into this.
msg206804 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-12-22 03:17
So can you tell me how this will make users' lives easier?  I don't really understand the issues involved.  But the only concrete thing I've seen mentioned is making testing easier, and that's not worth breaking feature freeze over.
msg206805 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-22 03:34
Yeah, I think we can safely leave this to 3.5.
msg206806 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-22 03:34
Right now say you have 2 module specs that are the same.  The only difference is that the 2 loaders are not the same instance (they were created separately with the same arguments, ergo equal).  The two specs will not compare as equal even though they are equal.

I expect users will find it surprising if they compare module.__spec__ to another spec that is basically the same (as described above) and it resolve to not equal.  I can see this as particularly vexing for importer writers that are switching over to the new spec-based APIs.

In my mind, the benefit of removing that unexpected (and aggravating) behavior outweighs the risk that someone is depending on identity-only comparision for the two loader types that are impacted by this change (which were both just added in 3.3).
msg206807 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-22 03:36
Importer writers are already used to __loader__ being annoying, and comparting specs for equality is unlikely to be a common thing (and easily worked around by comparing spec.origin instead)
msg206827 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-12-22 18:45
1. Is this patch going to change best practice for working with ModuleSpec?

2. If we delayed it to 3.5, will users have to ignore it to work around the deficiencies of the ModuleSpec implementation in 3.4?

I'm guessing the answer to both of these is "well, no, not really" simply because comparing ModuleSpec objects is not expected to be a regular operation.
msg206837 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-23 00:28
Yes, I think it will just make the third party idiom for testing that the
right module was imported to be to check spec.origin rather than comparing
specs directly. It's a nice-to-have, rather than something essential that
justifies breaking feature freeze.
msg206838 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-23 00:29
That is, I think the answer to both your questions is actually "Yes, but it
doesn't really matter due to the obscurity of the use case".
msg206883 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-12-24 01:37
I'm fine with this.  Thanks, Larry, for your attentiveness and diligence.
msg207297 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-04 18:06
So, not to yank your chain, but... I'm okay with checking this in.  Yes, we're already in beta, but ModuleSpec is brand new, and the sense I get is that this use case is obscure even for ModuleSpec.  The only installed base is beta 1 users, and given that this is new functionality...

Anyway.  I expect to tag late tonight, say in twelve hours.  Can you check it in before then?
msg207308 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-01-04 20:43
I'll commit it in a little while.  Thanks.
msg207316 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-04 22:13
New changeset a72a0e4dad20 by Eric Snow in branch 'default':
Issue #19927: Add __eq__ to path-based loaders in importlib.
http://hg.python.org/cpython/rev/a72a0e4dad20
msg207331 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-05 02:29
You broke buildbots.  Please fix.

http://buildbot.python.org/all/builders/AMD64%20FreeBSD%2010.0%203.x/builds/1389
msg207332 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-05 02:32
Hmm, hard to see how you caused that with the path loader change.  Still please take a quick look.

I fired off another build to see if it was a transient error, but that'll take a while.
msg207333 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-01-05 02:45
I'll take a look.  It could be something with #19713 or #19708 that also failed there.

The other failing buildbot for those 3 changesets is http://buildbot.python.org/all/builders/x86%20Windows7%203.x/builds/7800.
msg207339 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-01-05 03:20
The windows buildbot failure looks like a race condition in a test unrelated to my changes (see issue #20127).  I'm looking at the FreeBSD failure now.
msg207340 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-01-05 03:21
Which passed on the subsequent run...
msg207341 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-01-05 03:30
The FreeBSD failure happened in test_threading (apparently), where it was the last test to "finish".  In the passing run it finished 339/388 -- the seed was different (1253928 vs. 5389019).

This does not seem to be related to my 3 changesets.  I'm guessing it's one of those lingering race conditions we have sprinkled here and there.
History
Date User Action Args
2014-01-05 07:12:17eric.snowsetstatus: pending -> closed
2014-01-05 03:30:35eric.snowsetstatus: open -> pending

messages: + msg207341
2014-01-05 03:21:43eric.snowsetmessages: + msg207340
2014-01-05 03:20:18eric.snowsetmessages: + msg207339
2014-01-05 02:45:12eric.snowsetmessages: + msg207333
2014-01-05 02:32:17larrysetmessages: + msg207332
2014-01-05 02:29:13larrysetstatus: closed -> open
priority: normal -> high
messages: + msg207331

assignee: eric.snow
resolution: fixed ->
stage: resolved -> needs patch
2014-01-04 22:14:47eric.snowsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2014-01-04 22:13:50python-devsetnosy: + python-dev
messages: + msg207316
2014-01-04 20:43:08eric.snowsetmessages: + msg207308
versions: + Python 3.4, - Python 3.5
2014-01-04 18:06:16larrysetmessages: + msg207297
2013-12-24 01:37:22eric.snowsetmessages: + msg206883
2013-12-23 00:29:37ncoghlansetmessages: + msg206838
2013-12-23 00:28:00ncoghlansetmessages: + msg206837
2013-12-22 18:45:03larrysetmessages: + msg206827
2013-12-22 03:36:19ncoghlansetmessages: + msg206807
2013-12-22 03:34:26eric.snowsetmessages: + msg206806
2013-12-22 03:34:07ncoghlansetpriority: high -> normal

messages: + msg206805
versions: + Python 3.5, - Python 3.4
2013-12-22 03:17:23larrysetmessages: + msg206804
2013-12-22 03:10:39eric.snowsetmessages: + msg206803
2013-12-22 03:03:53ncoghlansetmessages: + msg206802
2013-12-21 19:41:46eric.snowsetmessages: + msg206771
2013-12-21 18:31:18larrysetmessages: + msg206767
2013-12-20 08:45:07eric.snowsetmessages: + msg206669
2013-12-10 03:16:39eric.snowsetfiles: - issue19927-loader-eq.diff
2013-12-10 03:16:23eric.snowsetfiles: + issue19927-loader-eq.diff

messages: + msg205768
2013-12-08 16:15:14brett.cannonsetmessages: + msg205577
2013-12-08 12:24:38larrysetmessages: + msg205553
2013-12-08 06:31:36ncoghlansetmessages: + msg205521
2013-12-08 06:16:12eric.snowsetfiles: + issue19927-loader-eq.diff
keywords: + patch
messages: + msg205518

stage: test needed -> patch review
2013-12-08 05:14:05eric.snowsetmessages: - msg205515
2013-12-08 05:13:37eric.snowsetnosy: + larry

messages: + msg205516
title: Path-based loaders lack a meaningful __eq__() implementation.ModuleSpec.__eq__() is highly sensitive to loader.__eq__() -> Path-based loaders lack a meaningful __eq__() implementation.
2013-12-08 05:08:13eric.snowcreate