Feature social say #522 #524

jeremylan · 2018-06-06T23:18:33Z

#522
Created entries in the config file pocs.yaml, handling code in core.py and social sinks classes for Twitter and Slack.

codecov · 2018-06-06T23:45:24Z

Codecov Report

Merging #524 into develop will increase coverage by 0.12%.
The diff coverage is 84%.

@@             Coverage Diff             @@
##           develop     #524      +/-   ##
===========================================
+ Coverage    70.33%   70.46%   +0.12%     
===========================================
  Files           62       64       +2     
  Lines         5418     5468      +50     
  Branches       752      759       +7     
===========================================
+ Hits          3811     3853      +42     
- Misses        1398     1406       +8     
  Partials       209      209

Impacted Files	Coverage Δ
pocs/utils/social_twitter.py	`81.81% <81.81%> (ø)`
pocs/utils/social_slack.py	`88.23% <88.23%> (ø)`
pocs/serial_handlers/protocol_arduinosimulator.py	`75.38% <0%> (ø)`	⬆️

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 aef07d5...0499f64. Read the comment docs.

jamessynge · 2018-06-08T02:11:42Z

Thanks Jeremy. I'll take a look this weekend... I've got a very busy week in-progress now.

jamessynge · 2018-06-10T17:40:39Z

I'd prefer to not have this integrated into core.py, but rather as a small binary (e.g. POCS/script/say_to_twitter.py), which takes command line args or can read from a yaml file as you've done, to configure the zeromq reading and the twitter writing. This makes it easier to test, and it doesn't further complicate the already complicated core.py?

Thoughts?

wtgee · 2018-06-10T19:08:01Z

Sorry, I've been traveling to and talking at a conference so haven't looked at this used but would tend to agree that this feature shouldn't be in core.py. I'll try to take a look later tonight but it's long days here. Cheers, Wilfred

…

On Sun, Jun 10, 2018, 12:40 James Synge ***@***.***> wrote: I'd prefer to not have this integrated into core.py, but rather as a small binary (e.g. POCS/script/say_to_twitter.py), which takes command line args or can read from a yaml file as you've done, to configure the zeromq reading and the twitter writing. This makes it easier to test, and it doesn't further complicate the already complicated core.py? Thoughts? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#524 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEUUJ8FKnCFX-bu8nKUro7Gg-Hn4uBpks5t7VoYgaJpZM4UdgpN> .

wtgee · 2018-06-10T19:10:27Z

Also, on quick glance, can you remove the camelCase? We're trying to maintain a consistency with pep8. Thanks!

…

On Sun, Jun 10, 2018, 14:07 Wilfred Tyler Gee ***@***.***> wrote: Sorry, I've been traveling to and talking at a conference so haven't looked at this used but would tend to agree that this feature shouldn't be in core.py. I'll try to take a look later tonight but it's long days here. Cheers, Wilfred On Sun, Jun 10, 2018, 12:40 James Synge ***@***.***> wrote: > I'd prefer to not have this integrated into core.py, but rather as a > small binary (e.g. POCS/script/say_to_twitter.py), which takes command line > args or can read from a yaml file as you've done, to configure the zeromq > reading and the twitter writing. This makes it easier to test, and it > doesn't further complicate the already complicated core.py? > > Thoughts? > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#524 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAEUUJ8FKnCFX-bu8nKUro7Gg-Hn4uBpks5t7VoYgaJpZM4UdgpN> > . >

jeremylan · 2018-06-10T23:28:23Z

Hi thanks for the comments.

Yes I can certainly have it a separate script. I left in core as there was already an asynchronous handler for commands so I reused the similar code and made it totally configurable from the config. Also the slack output was in core.

If I have it on a separate script, how should it get invoked? I.e. as a totally separate process invoked from command line or invoked by pocs_shell?

Regarding the camel case no probs, as it is passing the unit test for consistency I thought it was ok.

If you could give me guidance on the invocation question above I'll modify the PR accordingly.

Cheers

wtgee

Submitted here with a few comments. You were right to follow what was existing with the message processes, but they are slightly separate as some of those message loops need to interact with the running instance. The slack and twitter communication can be external as they mostly just need to receive the messages and then forward them on. I don't think we will ever move to having slack feed messages back to the system.

Also, the messaging items that we have currently are not really ideal and would probably also be removed from core anyway. They were inserted as part of a student project and it hasn't quite ever been cleaned up yet. For your new code it would be best for us implement it a little cleaner.

Sorry for the slow response. I'm at a conference in the US and it has my time fully occupied for this week.

wtgee · 2018-06-11T12:51:04Z

requirements.txt

@@ -22,3 +22,5 @@ scikit_image >= 0.12.3
 scipy >= 0.17.1
 transitions >= 0.4.0
 wcsaxes
+tweepy


Can you move this to alphabetical order?

wtgee · 2018-06-11T12:52:31Z

pocs/utils/social_slack.py

+    """
+    logger = get_root_logger()
+
+    def __init__(self, web_hook, output_timestamp):


Let's switch this to output_timestamp=False by default.

Did you want to address this?

This was actually changed to False by default

wtgee · 2018-06-11T12:58:27Z

pocs/utils/social_twitter.py

+            self.output_timestamp = output_timestamp
+
+        except tweepy.TweepError as e:
+            self.logger.error('Error connecting to Twitter. Err: {} - Message: {}'.format(e.args[0][0]['code'], e.args[0][0]['message']))


Probably just throw a warning here and save error for critical failures.

wtgee · 2018-06-11T12:59:09Z

pocs/utils/social_twitter.py

@@ -0,0 +1,32 @@
+import tweepy
+
+from pocs.utils import current_time


wtgee · 2018-06-11T12:59:35Z

pocs/utils/social_slack.py

@@ -0,0 +1,26 @@
+import requests
+
+from pocs.utils import current_time


wtgee · 2018-06-12T04:31:56Z

So I think we want to make this a separate script in the scripts dir (or perhaps just part of scripts/start_messaging_hub.py? @jamessynge?).

jeremylan · 2018-06-12T05:14:53Z

Thanks for the comments @wtgee

No problem about the response time, I'm also working at this when I can.
I will start cleaning up core.py and also implement the changes you commented on, no issue!
It's not a critical feature so no rush :)

Regarding the camel casing, I assume you mean the variables in code like socialConfig right? I'll change the names.

I'll wait for @jamessynge guidance, but I think I got the scripts architecture a little better now, there is a start_xxxx script in the startup directory and the real code (which I can bring over from core.py) in the run_xxxx script.

So I could really have start_social_messaging.sh and run_social_messaging.py.
Can I leave the config stuff in pocs.yaml or should I create a separate one?

Cheers and enjoy the conference.

wtgee · 2018-06-12T12:47:50Z

Regarding the camel casing, I assume you mean the variables in code like socialConfig right? I'll change the names.

Yup, the class names are fine. Relevant pep8 here.

I'll wait for @jamessynge guidance, but I think I got the scripts architecture a little better now, there is a start_xxxx script in the startup directory and the real code (which I can bring over from core.py) in the run_xxxx script.
So I could really have start_social_messaging.sh and run_social_messaging.py.

Yeah, sounds good about the above.

Can I leave the config stuff in pocs.yaml or should I create a separate one?

Let's keep in pocs.yaml for now but keep it commented out

Thanks!

jeremylan · 2018-06-16T06:45:47Z

Amended PR based on feedback @wtgee @jamessynge. Sorry it took a while.
I restored core.py to the version before my initial commit.
Hope this new changes are OK.
Thanks

jamessynge · 2018-06-17T17:27:39Z

pocs/utils/social_twitter.py

+
+            self.api = tweepy.API(auth)
+            self.output_timestamp = output_timestamp
+        except tweepy.TweepError as e:


Does this error mean that the object is unusable? If so, it would probably be best to raise the exception again.

Yes this could happen due to config error. I'll address this.

jamessynge · 2018-06-17T17:28:24Z

conf_files/pocs.yaml

+#         access_token_secret: [your_access_token_secret] 
+#     slack:
+#         webhook_url: [your_webhook_url]
+#         output_timestamp: False


Should output_timestamp also appear in the twitter config?

It can but I did not provide it in the template as Twitter will reject status updates which are considered duplicates. So if you start/stop/start your POCS instance for example, Twitter will reject the duplicated statuses without the timestamp.

Hence I left the possibility for the user to override in case he needs no timestamp but the recommended default is False

Interesting. If the updates are identical but not consecutive, are they still considered duplicates? I.e. if the messages are A, A, and A, I gather that only the first A is allowed. But what about A, B, A?

No that would not be allowed either.
The rules are fuzzy but they are getting stricter. I'm not super familiar with Twitter but there is some info here and here

Thanks for the clarification.

jamessynge · 2018-06-17T17:30:18Z

pocs/utils/social_slack.py

+
+class SocialSlack(object):
+
+    """Messaging class to output to Slack


Nit: please use punctuation. And in this case I'd end the docstring on that first line, e.g.:

"""Messaging class to output to Slack."""

Sorry this is my editor playing with quotes, I will fix this

jamessynge · 2018-06-17T17:30:30Z

pocs/utils/social_slack.py

+    """
+    logger = get_root_logger()
+
+    def __init__(self, web_hook, output_timestamp):


Did you want to address this?

jamessynge · 2018-06-17T17:41:14Z

pocs/utils/social_twitter.py

+    """
+    logger = get_root_logger()
+
+    def __init__(self, consumer_key, consumer_secret, access_token, access_token_secret, output_timestamp):


I wonder whether this class should just get a dict (from the yaml reader) representing the config inside the twitter block of the config file. That would allow the caller to be relatively independent of the set of configs needed by one of these classes.

That is certainly possible, however the style of coding in the other modules I saw is to handle the config in the 'main' module rather than the utils module. I think that is how PanMessaging works for example.

I thought passing the parameters rather than a dictionary was a little clearer as it's a sort of DI of the stuff this class needs

Let's not assume that we've been perfect with our previous code! ;-)

But this does remind me of another approach: pass in the dict as **kwargs so that they get expanded when passed into SocialTwitter or SocialSlack.

OK I'll look into this thanks

jamessynge · 2018-06-17T17:46:26Z

scripts/run_social_messaging.py

+        parser.print_help()
+        sys.exit(1)
+
+    # Initialise social sinks to None


English, rather than American, spelling of initialize. Just curious if you are from outside of the US.

Yes I am Sydney based. I can change this if it's a problem

No, not a problem, just curious.

jamessynge · 2018-06-17T17:52:35Z

scripts/run_social_messaging.py

+    if args.from_config:
+        config = load_config(config_files=['pocs'])
+
+        if 'social_accounts' in config:


Alternately, could do something like:

social_config = config.get('social_accounts', {}) twitter_config = social_config.get('twitter') if twitter_config: try: social_twitter = SocialTwitter(twitter_config) except SocialException as e: log...

dict.get(key, default_value=None) defaults to returning None, so you don't have to worry about testing if the key is present. And you can provide a default value other than None.

Ok that's great. I'm not familiar with Python hence I copied this style from somewhere in core.py. I can change to this as I also find it cleaner thanks.

As per my comment above, you can do:

social_twitter = SocialTwitter(**twitter_config)

Then declare SocialTwitter's ctor as:

def __init__(self, kwarg_one=some_default, kwarg_two=None, ...)

jamessynge · 2018-06-17T17:54:31Z

scripts/run_social_messaging.py

+            if social_config is not None and 'twitter' in social_config:
+                twitter_config = social_config['twitter']
+                if twitter_config is not None and \
+                        'consumer_key' in twitter_config and \


If you got with my suggestion earlier, this validation will move into the twitter class. And you could of course use a factory method rather than the constructor to do your validation, if that made things cleaner.

See my comment above about handling config in main module

jamessynge · 2018-06-17T17:55:42Z

scripts/run_social_messaging.py

+                    social_slack = SocialSlack(slack_config['webhook_url'],
+                                                    output_timestamp)
+        else:
+            arg_error('social_accounts setting not defined in config.')


I wouldn't emit an error for this, only if the config is actually bad.

jamessynge · 2018-06-17T17:57:51Z

scripts/run_social_messaging.py

+        else:
+            arg_error('social_accounts setting not defined in config.')
+
+    if not social_twitter and not social_slack:


Let's treat this as normal. That way our startup scripts can always include executing this script, and it will just exit if there are no social accounts specified, with a non-error message such as "No social messaging sinks defined, exiting."

jamessynge · 2018-06-17T17:59:34Z

Thanks very much for the adjustments, I think this is getting very close.

jamessynge · 2018-06-17T18:00:10Z

Oh, and can you think how to test this?

jamessynge · 2018-06-17T18:02:42Z

pocs/utils/social_slack.py

@@ -0,0 +1,25 @@
+import requests


I'm not familiar with the requests library. Where does that come from?

This was in @wtgee code he originally used to output to Slack.
See the original code here.
I just reused it.

@wtgee Can you tell us where this comes from? It doesn't appear to be standard Python.

It's not standard python but it's more straight-forward to use than urllib or urllib2. I was actually surprised we weren't already using it somewhere. It's a popular library so I don't have any issues including it.

We don't have this in requirements.txt. @wtgee do you know how we are getting it installed?

Let's put it in the requirements. We hadn't used this before so wasn't there.

jeremylan · 2018-06-17T22:31:59Z

Hi @jamessynge thanks for the review. Please see my individual comments inline.
I'll address your points with another commit as soon as I get some time.
Thanks

jeremylan · 2018-06-19T02:07:21Z

Hi @jamessynge, @wtgee

I just pushed another commit addressing (hopefully) all the points we discussed.

Regarding testing, I'm not too sure how to write an unit test for this. I guess we could mock config values and send some tweets/slacks from the unit test, however test accounts would be needed...

jamessynge · 2018-06-20T19:01:30Z

Thanks, I'll take a look soon. Regarding testing, I found these links with a quick search: https://bobbelderbos.com/2016/06/unittest-with-python/ https://github.com/python-glasgow/pythonglasgow/blob/master/tests/test_twitter.py

On Mon, Jun 18, 2018, 10:07 PM Luca ***@***.***> wrote: Hi @jamessynge <https://github.com/jamessynge>, @wtgee <https://github.com/wtgee> I just pushed another commit addressing (hopefully) all the points we discussed. Regarding testing, I'm not too sure how to write an unit test for this. I guess we could mock config values and send some tweets/slacks from the unit test, however test accounts would be needed... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#524 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYmUwN-MbrIzQJVz85erohb38MVIfTOks5t-FzbgaJpZM4UdgpN> .

On Jun 18, 2018 10:07 PM, "Luca" <[email protected]> wrote: Hi @jamessynge <https://github.com/jamessynge>, @wtgee <https://github.com/wtgee> I just pushed another commit addressing (hopefully) all the points we discussed. Regarding testing, I'm not too sure how to write an unit test for this. I guess we could mock config values and send some tweets/slacks from the unit test, however test accounts would be needed... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#524 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYmUwN-MbrIzQJVz85erohb38MVIfTOks5t-FzbgaJpZM4UdgpN> .

… sink classes

jeremylan · 2018-06-23T00:34:17Z

Hi @jamessynge

Thanks for the links about unit testing, they were useful. Python's mocking framework is very interesting.

I added the unit test module, it took me a little to finish as pytest does not support mock decorators and it puzzled me for a while. It all works using the with format.

I also made a small change in the sink classes to better handle empty parameters, and I removed inconsistent use of single and double quotes.

Thanks

jamessynge

LGTM, modulo a few nits.

jamessynge · 2018-06-28T18:26:51Z

conf_files/pocs.yaml

+#         access_token_secret: [your_access_token_secret] 
+#     slack:
+#         webhook_url: [your_webhook_url]
+#         output_timestamp: False


Thanks for the clarification.

jamessynge · 2018-06-28T18:33:44Z

pocs/tests/test_social_messaging.py

+
+
+def test_send_message_twitter_no_timestamp(twitter_config):
+    with unittest.mock.patch.dict(twitter_config, {'output_timestamp': False}):


FYI, for this kind of nested with you can put multiple with_items in a single with statement. See the docs here: https://docs.python.org/3/reference/compound_stmts.html#the-with-statement

Will change this thanks

jamessynge · 2018-06-28T18:35:23Z

pocs/utils/social_slack.py

@@ -0,0 +1,25 @@
+import requests


We don't have this in requirements.txt. @wtgee do you know how we are getting it installed?

jamessynge · 2018-06-28T18:35:54Z

pocs/tests/test_social_messaging.py

@@ -0,0 +1,103 @@
+import pytest


Great job on the tests, thanks.

jamessynge · 2018-06-28T18:37:36Z

pocs/utils/social_slack.py

+    def send_message(self, msg, timestamp):
+        try:
+            if self.output_timestamp:
+                post_msg = '{} - {}'.format(timestamp, msg)


FWIW, I think we might find it easier to consume these messages if the timestamp comes second, after msg. For example, on my phone I'd rather see timestamp truncated off than the msg.

jamessynge · 2018-06-28T18:37:56Z

pocs/utils/social_twitter.py

+    def send_message(self, msg, timestamp):
+        try:
+            if self.output_timestamp:
+                retStatus = self.api.update_status('{} - {}'.format(timestamp, msg))


Ditto here about order.

jamessynge · 2018-06-28T18:42:40Z

scripts/run_social_messaging.py

+    parser.add_argument(
+        '--from_config',
+        action='store_true',
+        help='Read social channels config from the pocs.yaml and pocs_local.yaml config files.')


At this point we don't really have any need for argparse, right? If you agree, I'd drop it until needed.

@jamessynge you mean because there is only one option or just because it is the default?

I'd prefer that if we parse any command line options we do it with ArgParse, even if it is only one argument. It's more consistent and ArgParse looks cleaner than using sys.argv.

However, what we haven't done is establish conventions on naming arguments from the command line. I think the standard would be to always do hypens instead of underscores on the command line, which ArgParse automatically adjusts. So --from-config on the command line becomes args.from_config.

So @wtgee, @jamessynge nothing to do here then right? BTW I lifted this code from run_messaging_hub.py

jamessynge

LGTM!

jeremylan added 2 commits June 7, 2018 08:59

First implementation social sinks for POCS messages

66f420c

Fixed code conventions to pass test

19f6935

wtgee requested review from wtgee and jamessynge June 6, 2018 23:37

wtgee reviewed Jun 12, 2018

View reviewed changes

Moved code to separate script. Amended based on feedback.

faa4ac9

jamessynge requested changes Jun 17, 2018

View reviewed changes

jamessynge reviewed Jun 17, 2018

View reviewed changes

Addressed review points and refactored sink classes

9af9492

Added unit tests for Social Messaging, improved parameter defaults in…

76ae63c

… sink classes

jamessynge requested changes Jun 28, 2018

View reviewed changes

Small changes from review

0499f64

jamessynge approved these changes Jul 1, 2018

View reviewed changes

jamessynge merged commit 7fa441f into panoptes:develop Jul 1, 2018

		@@ -0,0 +1,32 @@
		import tweepy

		from pocs.utils import current_time

		@@ -0,0 +1,26 @@
		import requests

		from pocs.utils import current_time


		class SocialSlack(object):

		"""Messaging class to output to Slack



		def test_send_message_twitter_no_timestamp(twitter_config):
		with unittest.mock.patch.dict(twitter_config, {'output_timestamp': False}):

Feature social say #522 #524

Feature social say #522 #524

Conversation

jeremylan commented Jun 6, 2018

codecov bot commented Jun 6, 2018 • edited Loading

Codecov Report

jamessynge commented Jun 8, 2018

jamessynge commented Jun 10, 2018

wtgee commented Jun 10, 2018 via email

wtgee commented Jun 10, 2018 via email

jeremylan commented Jun 10, 2018

wtgee 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

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

wtgee commented Jun 12, 2018

jeremylan commented Jun 12, 2018 • edited Loading

wtgee commented Jun 12, 2018

jeremylan commented Jun 16, 2018

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

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

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

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

jamessynge commented Jun 17, 2018

jamessynge commented Jun 17, 2018

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

jeremylan commented Jun 17, 2018

jeremylan commented Jun 19, 2018

jamessynge commented Jun 20, 2018 via email

jeremylan commented Jun 23, 2018 • edited Loading

jamessynge 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamessynge left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 6, 2018 •

edited

Loading

jeremylan commented Jun 12, 2018 •

edited

Loading

jeremylan commented Jun 23, 2018 •

edited

Loading