Insights 12 #38

robert-close · 2020-09-04T13:53:36Z

Adding method to allow users to send insight information from a python algorithm to the queue reader. I am not very experienced with Python, so please point out any areas for improvement or correction. This is a pretty simple flow and I could have just called the request.post from the client, but I feel it is cleaner to have the Insights class segregated.

…emoved the ability to collect insights before sending, now the every time the user invokes the collectInsights method, it will also send. This prevents any State issues with the algorithm.

… necessary.

robert-close · 2020-09-04T13:55:11Z

These test failures are not clear to me. Is it a 2.7 problem?

dinoboy197 · 2020-09-08T16:12:43Z

Algorithmia/insights.py

+
+class Insights:
+    # Example of correct insights:
+    # {


Would it be possible to use a more streamlined key / value pair structure? For example, what we've written up in the documentation proposal for Python so far? (Sorry I didn't get this to you last week):

client.report_insights({"cats_in_image": 4, "dogs_in_image": 7})

Got it, I had not seen that document before. I changed the method signatures as well

…insights1.enthalpy.click/developers/clients/python#publishing-algorithmia-insights

kennydaniel · 2020-09-08T17:26:07Z

Algorithmia/insights.py

+    #   {"aKey2":"aValue2"}
+    # }
+    def __init__(self, insights):
+        # TODO should we get the URL from a config?


I would like to see this use an environment variable with a fallback to localhost. First of all, when doing local dev, 9000 will often already be taken by play server (but this is expecting to hit a sidecar right?). Also, it's difficult to get users to update python client on a schedule, so having more flexibility from the start is valuable.

I created this Jira to set the env variable. https://algorithmia.atlassian.net/browse/INSIGHTS-36. This code has been updated to default to the localhost:9000 if it is not set.

kennydaniel · 2020-09-08T17:26:49Z

Test/algo_test.py

-        result = self.client.algo('util/Echo').pipe(bytearray('foo','utf-8'))
-        self.assertEquals('binary', result.metadata.content_type)
-        self.assertEquals(bytearray('foo','utf-8'), result.result)
+# todo fix this


This seems unrelated to the insights code, why did the test need to be commented out?

This test is failing in the pipeline, I am not sure what is causing it.

robert-close · 2020-09-10T19:40:06Z

The build is failing for an unrelated test. Any help would be appreciated.

dinoboy197 · 2020-09-15T00:28:34Z

@kennydaniel the failure in the tests seems to be:

__________________________ AlgoTest.test_call_binary ___________________________

self = <Test.algo_test.AlgoTest testMethod=test_call_binary>

    def test_call_binary(self):
        result = self.client.algo('util/Echo').pipe(bytearray('foo','utf-8'))
        self.assertEquals('binary', result.metadata.content_type)
>       self.assertEquals(bytearray('foo','utf-8'), result.result)
E       AssertionError: bytearray(b'foo') != 'Zm9v'

Given that you and Ben worked on this recently, would you have any idea what this might be caused by?

dinoboy197

I'm curious about that failing test; I can't see how it's related to your code, but I'm hoping Kenny might have a thought on that?

dinoboy197 · 2020-09-15T00:29:22Z

setup.py

 import os

 from setuptools import setup

 setup(
    name='algorithmia',
-    version='1.4.0',
+    version='1.4.4',


Since we're adding new functionality, it seems like a minor version bump (1.4.0 -> 1.5.0) would be more appropriate given semver - thoughts @ZachEddy ?

Yep, I agree. Bumping the minor version makes sense here.

Sidenote: weirdly, this is the only Slack notification that GitHub sent me for this pull request, so I've been missing tons of activity 😕

I wanted to wait to do that bump until we are fully tested as we can only use 1.5.0 once in Pypi, but I will add this to the commit here. I can change it locally when I push to Pypi for testing.

dinoboy197 · 2020-09-15T00:29:48Z

setup.py

@@ -1,10 +1,11 @@
+import distutils


Is distutils a new library that is needed for setup but somehow wasn't before?

Sorry, that was a left over from an unneeded change I tried.

ZachEddy · 2020-09-15T16:24:20Z

Algorithmia/insights.py

+        headers['Content-Type'] = 'application/json'
+        AQR_URL = os.getenv('ALGORITHM_QUEUE_READER_URL') or "http://localhost:9000"
+
+        requests.post(AQR_URL+"/insights", data=json.dumps(insights).encode('utf-8'), headers=headers)


Something that came out of the design review: the endpoint in AQR should be prefixed with /v1/ so we can safely make breaking API changes without breaking the v1.X.X client code.

I still need to modify AQR and add the prefix but that is a small change, so let's go ahead and make the change in this PR.

/insights --> /v1/insights

kennydaniel · 2020-09-22T16:30:23Z

Algorithmia/client.py

@@ -40,6 +41,11 @@ def dir(self, dataUrl):
        if dataUrl.startswith('file://'): return LocalDataDirectory(self, dataUrl)
        else: return DataDirectory(self, dataUrl)

+    # Used to send insight data to Algorithm Queue Reader in cluster
+    def report_insights(self, insights):
+        return Insights(insights)


Two comments:

why are we returning theInsights object? Seems like void return type makes more sense here.

I think this should be wrapped in a try/catch so make local running easier. Right now I think it will throw exception if running locally with insights.

dinoboy197 · 2020-09-28T18:01:35Z

Will rebuild this PR once AQR with binary payload fix is deployed to production public marketplace.

dinoboy197 · 2020-10-05T17:08:32Z

@algorithmiaio/developer-technologies-contractors ping for a review please!

dinoboy197 · 2020-10-06T17:44:07Z

Thanks for your approval @John-Bragg ! You can just leave the PRs open once you approve them because we use Github to merge the changes.

dinoboy197 · 2020-10-06T17:44:27Z

@robert-close ready for you to merge!

robert-close added 6 commits September 2, 2020 15:06

INSIGHTS-12 Initial structure of insight functionality

2be3a68

INSIGHTS-12 Added todo statements

601e367

INSIGHTS-12 Moved Insights out of client

ea1741b

INSIGHTS-12 Adjusted insight methods to reside in the client class. R…

6e53c18

…emoved the ability to collect insights before sending, now the every time the user invokes the collectInsights method, it will also send. This prevents any State issues with the algorithm.

INSIGHTS-12 Added a todo. Tests fail for unknown reasons at this time

6b84190

INSIGHTS-12 Fixed method call. Added a todo to get url from config if…

0cf1255

… necessary.

robert-close requested review from dinoboy197, kennydaniel and ZachEddy September 4, 2020 13:54

robert-close added 4 commits September 4, 2020 09:57

INSIGHTS-12 Fixed method call.

46ad61c

INSIGHTS-12 added json serialization. might not be needed

5cc049e

INSIGHTS-12 commented test temporarily

d53f8cc

INSIGHTS-12 comment updates and json .encode change

d984e63

dinoboy197 reviewed Sep 8, 2020

View reviewed changes

robert-close added 2 commits September 8, 2020 12:58

INSIGHTS-12 comment update

80fb4c2

INSIGHTS-12 changed method signatures to match documentation https://…

1054514

…insights1.enthalpy.click/developers/clients/python#publishing-algorithmia-insights

kennydaniel reviewed Sep 8, 2020

View reviewed changes

robert-close added 3 commits September 9, 2020 09:38

INSIGHTS-12 Added system property for queue reader url

0d9c005

INSIGHTS-12 Fixed URL to not be https

71313fd

INSIGHTS-12 minor version update

0f2a377

robert-close marked this pull request as ready for review September 10, 2020 18:51

robert-close added 3 commits September 10, 2020 14:52

INSIGHTS-12 revert change

c5324a0

INSIGHTS-12 removed todo

3884857

INSIGHTS-12 uncommented test. May start failing again in the pipeline.

9b15069

robert-close requested a review from kennydaniel September 10, 2020 18:55

INSIGHTS-12 commented test.

d429304

dinoboy197 reviewed Sep 15, 2020

View reviewed changes

INSIGHTS-12 changed version. Removed unused import.

3e3a3ec

ZachEddy reviewed Sep 15, 2020

View reviewed changes

robert-close and others added 3 commits September 15, 2020 13:15

INSIGHTS-12 changed url to include /v1/

a0637d2

INSIGHTS-12 Uncommented tests

e2ac10f

Update Insights payload per AQR contract

fa5601c

kennydaniel reviewed Sep 22, 2020

View reviewed changes

INSIGHTS-36 Updated ENV Variable to correct value

3757236

Skip broken test; will fix in DEV-75

5173e73

dinoboy197 requested a review from a team October 2, 2020 23:53

John-Bragg approved these changes Oct 6, 2020

View reviewed changes

John-Bragg closed this Oct 6, 2020

dinoboy197 reopened this Oct 6, 2020

robert-close merged commit 1d0b5b3 into master Oct 6, 2020

robert-close deleted the INSIGHTS-12 branch October 6, 2020 18:02

Insights 12 #38

Insights 12 #38

Uh oh!

Conversation

robert-close commented Sep 4, 2020

Uh oh!

robert-close commented Sep 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robert-close commented Sep 10, 2020

Uh oh!

dinoboy197 commented Sep 15, 2020

Uh oh!

dinoboy197 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robert-close Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dinoboy197 commented Sep 28, 2020

Uh oh!

dinoboy197 commented Oct 5, 2020

Uh oh!

dinoboy197 commented Oct 6, 2020

Uh oh!

dinoboy197 commented Oct 6, 2020

Uh oh!

Uh oh!

robert-close Sep 15, 2020 •

edited

Loading