Throw exception if load() is called on instance rather than the class #889

aayux · 2016-09-26T19:38:10Z

Addresses one out of four issues in #692

gojomo · 2016-09-27T00:09:22Z

I believe a more compact fix, with less meta-method-juggling in multiple places, would be possible and desirable here. Can the load/load_word2vec_format methods just detect is their first parameter (cls) isn't the expected class-object?

aayux · 2016-09-27T03:57:19Z

@gojomo I'd have liked that too, but it appears that the method does not differentiate between class or self once the call is made.

gojomo · 2016-09-27T04:34:59Z

When it's called on an instance, isn't the first parameter (cls) an instance rather than the class-object?

tmylk · 2016-09-27T04:57:20Z

gensim/models/word2vec.py

+
+def load(self, *args, **kwargs):
+    logger.warn('Load was called on instance. Calling on class instead')
+    Word2Vec.load(*args, **kwargs)


This is not expected behaviour.
Expected behaviour is to print warning but still run the instance method.

I'd suggest that best would be to error – this method doesn't modify an instance in-place, so the "one and only one right way to do it" is to call it is via the class.

aayux · 2016-09-27T08:41:32Z

@gojomo when calling on instance, classmethod takes the calling instance's class as the hidden parameter.

cls is of type class-object: as a call to type (cls) will show.

tmylk

Move methods inside the instance. Link to code showing the first parameter.

tmylk · 2016-09-30T02:31:14Z

gensim/models/word2vec.py

@@ -452,6 +453,9 @@ def __init__(
        self.total_train_time = 0
        self.sorted_vocab = sorted_vocab
        self.batch_words = batch_words
+
+        self.load = methodize(load, self)


Did you mean self.load = methodize(self.load, self) ?
it is very confusing to assign self.load and then define def load

This was done on purpose. Please see here for reference.

MethodType binds the method to the calling instance. What I'm doing here is something like overloading load based on whether it is called on instance or class.

So the argument load to methodize is the name of the function that is to be bound.

tmylk · 2016-09-30T02:36:44Z

gensim/models/word2vec.py

@@ -1869,3 +1873,15 @@ def __iter__(self):
        model.accuracy(args.accuracy)

    logger.info("finished running %s", program)
+
+def methodize(func, instance):


These methods are defined outside of the class. Maybe that is why you see no difference in calling these methods and class methods? Can you link to a gist of the code you use to test the first parameter?

methods are defined outside of the class

This was done on purpose. Please see here for reference.

MethodType binds the method to the calling instance. What I'm doing here is something like overloading load based on whether it is called on instance or class.

So the argument load to methodize is the name of the function that is to be bound.

My latest build is failing on Python 2 for some reason, I'm working on that right now. Once that's done, I'll write the tests and merge it back with develop.

gojomo · 2016-10-03T01:47:51Z

Aha, sorry, I thought an obj.func_name() invocation always put obj in as the 1st argument.

This sort of warning would be a nice convenience for explaining a common user error that's been asked-about up a bunch of times (and probably many more times where the user eventually figured it out for themselves)... but it wouldn't justify much extra code or meta-method complexity.

How about if instead, there's a generic utility function (taking any number of positional/keyword params) that just throws an error to the effect of, "this method name should only be called on the class object" – and that function is installed on instances in the relevant method names? That could be a smaller and clearer way to catch the error.

aayux · 2016-10-24T16:56:29Z

@gojomo you're right, this is a very inefficient way of doing things.

What if I use inspect to get the calling frame and then create a utility that basically parses through the string to figure out if the function was called using Word2Vec.load etc.?

Issue-889

gojomo · 2016-10-24T19:07:54Z

This approach seems a bit more focused. But my concern is still complicating the code with advanced meta-features for such a minor convenience API warning.

(Separate observations: as SaveLoad might even now be used elsewhere, having a list of acceptable-known-subclasses coded inside its method is suboptimal – it shouldn't have to know all its subclass names. Also, at the moment post-upstream-merge, this PR diff is showing other unrelated changes as well – maybe a Github issue or your catchup-merge wasn't to current state of develop?)

aayux · 2016-10-25T05:21:44Z

@gojomo any solution we find for this will require complicating of the code to some extent. We've exhausted a lot of possible options.

Clearly, though, this approach has much fewer meta-features as compared to others.

as SaveLoad might even now be used elsewhere, having a list of acceptable-known-subclasses coded inside its method is suboptimal – it shouldn't have to know all its subclass names.

This is a valid concer. Do you have any recommendations?

maybe a Github issue or your catchup-merge wasn't to current state of develop?

It's the latter – I was editing in a different branch which was up to date with master. I'd forgotten about that.

gojomo · 2016-10-25T07:33:08Z

The suggestion in my 20161002 comment might be minimal in terms of line-count and magic. Roughly (not tested – I think this would work):

def call_on_class_only(*args, **kwargs):
    logger.warn('this method name should only be called on class objects')

(in Word2Vec/etc)

def __init__(...):
    ...
    self.load = call_on_class_only
    ...

Though perhaps also, if iterating over the @classmethods is easy, there'd be a utility function to auto-perform this replacement, so the impact in Word2Vec.__init__ (etc) would just be:

...
hide_class_only_methods(self)
...

(Essentially, this would be a slightly-slimmer refinement of your first approach.)

modified: gensim/test/test_word2vec.py

gojomo · 2016-10-31T19:57:14Z

Changes look good to me, as soon as tests are sensible and pass. Thanks for patiently sticking with this!

modified: gensim/test/test_word2vec.py

piskvorky · 2016-11-03T08:35:13Z

gensim/utils.py

@@ -217,6 +217,10 @@ def any2unicode(text, encoding='utf8', errors='strict'):
    return unicode(text, encoding, errors=errors)
 to_unicode = any2unicode

+def call_on_class_only(*args, **kwargs):
+    """Warns when load methods are called on instance"""
+    logger.warn('This method should be called on a class object.')


We don't want to be failing quietly (or, if logging not configured, in complete silence), ignoring the user's explicit request.

Exception is more appropriate.

Yes, I should have put an exception in my example, so that a coder making this error knows to change their calling code.

@Dust0x please fix.

piskvorky · 2016-11-03T08:40:33Z

gensim/models/word2vec.py

@@ -432,6 +432,8 @@ def __init__(

        """

+        self.load = call_on_class_only


But this will leave the attribute function unbound, leading to potential confusion / serialization issues.

Proper way would be to bind it properly with types.MethodType or somesuch.

If the serialization tests pass, and the exception message is understandable if it's ever called (by mistake), I don't see what other clarity would come from binding the method. That'd just make it look more like an instance-specific operation... which it specifically is not.

@piskvorky, that is what I did at the beginning. Apart from the points @gojomo raises, there are some issues with the way types.MethodTypes is handled in Python versions 2 and 3.

tmylk · 2016-11-08T18:06:42Z

gensim/test/test_word2vec.py

+        self.assertTrue(warning in str(l))
+
+    @log_capture()
+    def testLoadWord2VecOnClassWarning(self):


why is this second test necessary? The first test above is enough.

Yes, you're right. I'll remove that.

piskvorky · 2016-12-03T02:00:47Z

gensim/test/test_doc2vec.py

+        load_on_instance = doc2vec.Doc2Vec()
+        model = load_on_instance.load(testfile())
+        warning = "Load methods should be called on a class object."
+        self.assertTrue(warning in str(l))


What is this l, where does it come from?

Not a good name for a variable.

Unfortunately it seems to be the 'usual' name for the LogCapture objects provided by the testfixtures package @log_capture decorator. (See for example: https://testfixtures.readthedocs.io/en/latest/logging.html#the-decorator) But, that would seem to require an l parameter be added to the method, which I don't see here... so a bit baffled that this could work.

No of course it wouldn't work. Sorry, I was trying something you weren't supposed to see this. I should've done this in a separate branch.

piskvorky · 2016-12-03T02:01:08Z

gensim/test/test_word2vec.py

+        binary_model = word2vec.Word2Vec()
+        load_on_instance = binary_model.load_word2vec_format(testfile(), binary=True)
+        warning = "Load methods should be called on a class object."
+        self.assertTrue(warning in str(l))


Dtto -- what is l?

piskvorky · 2016-12-03T02:01:36Z

gensim/utils.py

@@ -217,6 +217,10 @@ def any2unicode(text, encoding='utf8', errors='strict'):
    return unicode(text, encoding, errors=errors)
 to_unicode = any2unicode

+def call_on_class_only(*args, **kwargs):
+    """Warns when load methods are called on instance"""
+    logger.warn('This method should be called on a class object.')


@Dust0x please fix.

Finishing up with tests for 889

aayux · 2016-12-04T14:48:37Z

Method call_on_class_only now raises AttributeError as requested.
Tests for load on instance completed.
CI passed.

Sorry for the delay. Request to review.

aayux added 4 commits September 27, 2016 01:04

Update word2vec.py

25fc004

Update word2vec.py

58976d8

Update word2vec.py

cf34e2d

Update doc2vec.py

773a75e

aayux changed the title ~~Raise warning if calling load() on an instance rather than the class~~ [WIP] Raise warning if calling load() on an instance rather than the class Sep 26, 2016

tmylk reviewed Sep 27, 2016

View reviewed changes

aayux added 2 commits September 29, 2016 19:35

Update doc2vec.py

96d65ec

Update word2vec.py

d91b99d

tmylk suggested changes Sep 30, 2016

View reviewed changes

tmylk added bug Issue described a bug difficulty medium Medium issue: required good gensim understanding & python skills labels Oct 4, 2016

aayux added 6 commits October 24, 2016 23:12

Update utils.py

c156c79

Update word2vec.py

60d42a3

Update utils.py

cc7e3e1

Update word2vec.py

a0ea2b4

Update doc2vec.py

2d85ef5

Merge pull request #1 from dust0x/Issue-889

ac0b0f3

Issue-889

aayux added 4 commits October 26, 2016 14:11

Update doc2vec.py

770a277

Update word2vec.py

772cdcd

Update utils.py

4c4ec8c

Update word2vec.py

7173993

aayux added 3 commits November 1, 2016 01:00

modified: gensim/test/test_doc2vec.py

cec0a1b

modified: gensim/test/test_word2vec.py

modified: gensim/test/test_doc2vec.py

c09e4d8

modified: gensim/test/test_word2vec.py

modified: gensim/test/test_doc2vec.py

ea89bd5

modified: gensim/test/test_word2vec.py

aayux added 2 commits November 1, 2016 01:36

modified: gensim/test/test_doc2vec.py

5d743f1

modified: gensim/test/test_word2vec.py

modified: gensim/test/test_doc2vec.py

d045bd2

modified: gensim/test/test_word2vec.py

piskvorky reviewed Nov 3, 2016

View reviewed changes

tmylk reviewed Nov 8, 2016

View reviewed changes

piskvorky requested changes Dec 3, 2016

View reviewed changes

aayux added 7 commits December 4, 2016 16:49

Merge https://github.com/RaRe-Technologies/gensim into develop

07b3700

Update test_word2vec.py

40f2a50

Update test_doc2vec.py

43c7b08

Update test_doc2vec.py

b50d60a

Update test_word2vec.py

eba33bc

Update doc2vec.py

6ffacdf

Update doc2vec.py

f0137b8

aayux changed the title ~~[WIP] Raise warning if calling load() on an instance rather than the class~~ Throw exception if load is called on instance rather than the class Dec 4, 2016

aayux changed the title ~~Throw exception if load is called on instance rather than the class~~ Throw exception if load() is called on instance rather than the class Dec 4, 2016

aayux added 6 commits December 4, 2016 18:26

Update test_word2vec.py

3bf25fc

Update test_word2vec.py

7519519

Update test_word2vec.py

2d28b03

Update test_doc2vec.py

19d2035

Update test_word2vec.py

e68bb13

Merge pull request #2 from dust0x/tests

37c9149

Finishing up with tests for 889

gojomo mentioned this pull request Dec 17, 2016

Warning if vocabulary is entirely single character elements in doc2vec #692

Closed

Merge branch 'develop' into develop

d9b45c3

tmylk merged commit 804825e into piskvorky:develop Dec 22, 2016

gojomo mentioned this pull request Feb 3, 2017

Loading models generated by other version of gensim #1134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw exception if load() is called on instance rather than the class #889

Throw exception if load() is called on instance rather than the class #889

aayux commented Sep 26, 2016 •

edited

Loading

gojomo commented Sep 27, 2016

aayux commented Sep 27, 2016 •

edited

Loading

gojomo commented Sep 27, 2016 •

edited

Loading

tmylk Sep 27, 2016

gojomo Sep 27, 2016

aayux commented Sep 27, 2016

tmylk left a comment

tmylk Sep 30, 2016

aayux Sep 30, 2016

tmylk Sep 30, 2016

aayux Sep 30, 2016 •

edited

Loading

aayux Sep 30, 2016 •

edited

Loading

gojomo commented Oct 3, 2016

aayux commented Oct 24, 2016 •

edited

Loading

gojomo commented Oct 24, 2016

aayux commented Oct 25, 2016 •

edited

Loading

gojomo commented Oct 25, 2016

gojomo commented Oct 31, 2016

piskvorky Nov 3, 2016 •

edited

Loading

gojomo Nov 3, 2016

piskvorky Dec 3, 2016

piskvorky Nov 3, 2016 •

edited

Loading

gojomo Nov 3, 2016

aayux Nov 30, 2016

tmylk Nov 8, 2016

aayux Nov 30, 2016

piskvorky Dec 3, 2016

gojomo Dec 3, 2016

aayux Dec 3, 2016 •

edited

Loading

piskvorky Dec 3, 2016

piskvorky Dec 3, 2016

aayux commented Dec 4, 2016

		@@ -432,6 +432,8 @@ def __init__(

		"""

		self.load = call_on_class_only

Throw exception if load() is called on instance rather than the class #889

Throw exception if load() is called on instance rather than the class #889

Conversation

aayux commented Sep 26, 2016 • edited Loading

gojomo commented Sep 27, 2016

aayux commented Sep 27, 2016 • edited Loading

gojomo commented Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayux commented Sep 27, 2016

tmylk 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

aayux Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

aayux Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

gojomo commented Oct 3, 2016

aayux commented Oct 24, 2016 • edited Loading

gojomo commented Oct 24, 2016

aayux commented Oct 25, 2016 • edited Loading

gojomo commented Oct 25, 2016

gojomo commented Oct 31, 2016

piskvorky Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piskvorky Nov 3, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayux Dec 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayux commented Dec 4, 2016

aayux commented Sep 26, 2016 •

edited

Loading

aayux commented Sep 27, 2016 •

edited

Loading

gojomo commented Sep 27, 2016 •

edited

Loading

aayux Sep 30, 2016 •

edited

Loading

aayux Sep 30, 2016 •

edited

Loading

aayux commented Oct 24, 2016 •

edited

Loading

aayux commented Oct 25, 2016 •

edited

Loading

piskvorky Nov 3, 2016 •

edited

Loading

piskvorky Nov 3, 2016 •

edited

Loading

aayux Dec 3, 2016 •

edited

Loading