Review usage of arrow.get() for Arrow 0.15.0 #313

davidag · 2019-09-10T16:17:48Z

Arrow 0.15.0 has been recently released and the warning messages mentioned in #296 have disappeared.

After reading the comments in that issue and reviewing the code, it seems to me that only user date input can be affected for some commands. For example, if someone relied on using timestamps in the command line, errors will probably be raised with Arrow 0.15.0.

@ryanmjacobs It would be great if you could review my changes and compare them with those you made a month ago 😌

Changes:

Reduce the number of uses of arrow.get() related to spans in reports.
Unify catched exceptions when calling arrow.get().
Include Frame.updated_at parsing inside try/catch just like
start/stop.

davidag · 2019-09-11T11:53:59Z

Of course, a review from @k4nar would be great, as much of this code is his! ☺️

k4nar

This looks good to me, but shouldn't we pin arrow to >0.15 in the setup then?

davidag · 2019-09-11T14:49:13Z

@k4nar I think that would be great, not because these changes require Arrow 0.15.0 (they should work fine with older Arrow versions), but to take advantage of all the fixes and improvements in that library. If you agree with this, I'll change the requirements.txt to arrow >= 0.15.0.

k4nar · 2019-09-11T14:53:41Z

Ah ok, I thought the changes were not backwards compatible. Then yes, updating the requirements.txt (and not the setup.py) should be enough 👍 .

davidag · 2019-09-11T15:31:44Z

It looks like setup.py gets the requirements from requirements.txt so it would be effectively upgraded too. Could that be a problem with distribution/packaging dependencies?

jmaupetit · 2019-09-11T16:30:27Z

This is a strong requirement that might break many users installation. If it's not required, I would not limit requirements. But we should mention it in the installation docs. WDYT?

davidag · 2019-09-11T18:11:13Z

This is a strong requirement that might break many users installation. If it's not required, I would not limit requirements.

Perfect, let's keep the requirements as they are.

But we should mention it in the installation docs. WDYT?

I'm not sure I understand what should be mentioned in the installation docs. I can't find any mention to requirementes or dependencies 🤷‍♂️

jmaupetit · 2019-09-11T19:04:24Z

I would have wrote something in the single user installation section, but it will make things complicated when it's not required. Forget it.

jmaupetit · 2019-09-13T15:53:35Z

I think these changes are worth mentioning in the CHANGELOG. WDYT?

davidag · 2019-09-13T19:33:02Z

I think these changes are worth mentioning in the CHANGELOG. WDYT?

Sure! I've just added a line 👍

It would be ideal that @ryanmjacobs reviewed and added his changes before merging this, but of course, he could do that a posteriori if you think this can be merged as is.

ryanmjacobs · 2019-09-13T22:18:55Z

Hey nice work David! I'm glad someone is finally making a move on this issue. I haven't really looked at the codebase since over a month ago. But I'll double-check everything and add mine tonight. You'll have a LGTM by tomorrow morning :)

…

On Fri, Sep 13, 2019 at 12:33 PM David Alfonso ***@***.***> wrote: I think these changes are worth mentioning in the CHANGELOG. WDYT? Sure! I've just added a line 👍 It would be ideal that @ryanmjacobs <https://github.com/ryanmjacobs> reviewed and added his changes before merging this, but of course, he could do that a posteriori if you think this can be merged as is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#313>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABAN46NBJA6Q47FJSQL4GA3QJPTHBANCNFSM4IVJ2B4Q> .

- Reduce the number of uses of arrow.get() related to spans in reports. - Unify catched exceptions when calling arrow.get(). - Include 'Frame.updated_at' parsing inside try/catch just like start/stop.

davidag · 2019-09-16T08:15:26Z

@ryanmjacobs Thank you! I think it's worth waiting for your changes then, even if it takes a bit more time, don't worry 👍

ryanmjacobs · 2019-09-17T23:24:53Z

@davidag Great, I've set aside time to rebase my changes tonight. I'll ping you in this thread when it's done.

ryanmjacobs · 2019-09-18T01:29:34Z

Okay, I've pushed my old changes... (I guess I never committed them...)

Looks like the only change I made was this: ryanmjacobs@80363e6. However, I like what you are doing more -- which is reducing arrow.get() usage altogether.

I've run setup.py install on my system using your repository. I see no errors with watson start test, watson stop, watson sync, and then watson log -a. Prior to these changes, I was getting errors on watson sync and watson log. All of my new and prior log data looks fine (correct datetimes and everything).

I might be totally wrong on this part. But if it's fine, then I think we are good to merge.

My only worry is watson report --csv, which I think was introduced after 1.7.0. I see that you've made changes to utils.py in the CSV function used for report() (https://github.com/TailorDev/Watson/pull/313/files#diff-53886fd7d4ba12dfad3dbb6c9716ca57L381).

Anyways, until today, I've been running 1.7.0. watson report -a looks fine to me, but watson report -a --csv gives me this. It looks funny, but maybe that's what the output is supposed to look like.

$ watson report -a --csv
from,to,project,tag,time
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,,194312.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,SearchBox,11778.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,app-js-overhaul,46644.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,debug-integration-whitescreen,101.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,demo-route,1157.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,deploy-remote-clone,2870.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,error-modal,1091.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,fix-dateslider-after-overhaul,1259.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,fix-remove-integration,4797.0
1970-01-01 00:00:00,2019-09-17 23:59:59,redacted,gridbuilder-streamline,105833.0
...

While watson report -a gives me this:

$ watson report -a
redacted - 53h 58m 32s
        [SearchBox  3h 16m 18s]
        [app-js-overhaul 12h 57m 24s]
        [debug-integration-whitescreen     01m 41s]
        [demo-route     19m 17s]
        [deploy-remote-clone     47m 50s]
        [error-modal     18m 11s]
        [fix-dateslider-after-overhaul     20m 59s]
        [fix-remove-integration  1h 19m 57s]
        [gridbuilder-streamline 29h 23m 53s]
        [local-backend     13m 00s]
        [onboarding-cleanup     24m 30s]
        [onboarding-debug  1h 29m 58s]
...

edit: My gut is telling me that it's supposed to look like that. But I'm just not 100% sure.

davidag · 2019-09-18T10:06:31Z

@ryanmjacobs Thank you for your comment, it's great you could test my changes and provide yours!

After careful reading, I'm not sure I understand your concerns with the output of watson report -a --csv.

When I developed the --csv option I got inspiration from --json, where the output times are in seconds, which made total sense to allow other tools to easily parse and transform the generated csv/json dump. You could compare the output using both formats.

If what worries you is that the end of the interval is 2019-09-17 23:59:59 instead of 2019-09-16 00:00:00, I think the former is correct. You might want to look at class Span in frames.py, which is used to build the span in watson.report().

Please let me know if I'm missing something!

ryanmjacobs · 2019-09-18T10:11:07Z

Oh, okay. The lines starting with `1970-01-01 00:00:00` looked concerning to me at first. They looked like uninitialized epochs, so I was hesitant. But now I see that it's probably okay. You're just setting a timerange that encompasses everything.

…

On Wed, Sep 18, 2019 at 3:06 AM David Alfonso ***@***.***> wrote: @ryanmjacobs <https://github.com/ryanmjacobs> Thank you for your comment, it's great you could test my changes and provide yours! After careful reading, I'm not sure I understand your concerns with the output of watson report -a --csv. When I developed the --csv option I got inspiration from --json, where the output times are *in seconds*, which made total sense to allow other tools to easily parse and transform the generated csv/json dump. You could compare the output using both formats. If what worries you is that the end of the interval is 2019-09-17 23:59:59 instead of 2019-09-16 00:00:00, I think the former is correct. You might want to look at class Span in frames.py, which is used to build the span in watson.report(). Please let me know if I'm missing something! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#313>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABAN46NCI4IFOMWWY3L54JTQKH4SRANCNFSM4IVJ2B4Q> .

davidag · 2019-09-18T10:15:31Z

@ryanmjacobs Yes, I think that's normal too, thank you again!

@jmaupetit I guess this is ready to merge 🎉

jmaupetit added the improvement label Sep 11, 2019

k4nar reviewed Sep 11, 2019

View reviewed changes

davidag force-pushed the review-arrow-get-usage branch from e5993ad to b59e8f3 Compare September 13, 2019 14:47

davidag force-pushed the review-arrow-get-usage branch from b59e8f3 to 121aa11 Compare September 13, 2019 19:26

Review usage of arrow.get() for Arrow 0.15.0

7e62cfa

- Reduce the number of uses of arrow.get() related to spans in reports. - Unify catched exceptions when calling arrow.get(). - Include 'Frame.updated_at' parsing inside try/catch just like start/stop.

davidag force-pushed the review-arrow-get-usage branch from 121aa11 to 7e62cfa Compare September 14, 2019 20:48

ryanmjacobs mentioned this pull request Sep 18, 2019

ArrowParseWarning, for the .get() method #296

Closed

jmaupetit approved these changes Sep 18, 2019

View reviewed changes

jmaupetit merged commit 297fd7f into jazzband:master Sep 18, 2019

davidag deleted the review-arrow-get-usage branch September 18, 2019 14:21

ericguirbal mentioned this pull request Oct 16, 2019

The report and aggregate commands with --json option don't work any more #329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review usage of arrow.get() for Arrow 0.15.0 #313

Review usage of arrow.get() for Arrow 0.15.0 #313

davidag commented Sep 10, 2019 •

edited

Loading

davidag commented Sep 11, 2019

k4nar left a comment

davidag commented Sep 11, 2019 •

edited

Loading

k4nar commented Sep 11, 2019

davidag commented Sep 11, 2019

jmaupetit commented Sep 11, 2019

davidag commented Sep 11, 2019

jmaupetit commented Sep 11, 2019

jmaupetit commented Sep 13, 2019

davidag commented Sep 13, 2019

ryanmjacobs commented Sep 13, 2019 via email

davidag commented Sep 16, 2019

ryanmjacobs commented Sep 17, 2019

ryanmjacobs commented Sep 18, 2019 •

edited

Loading

davidag commented Sep 18, 2019

ryanmjacobs commented Sep 18, 2019 via email

davidag commented Sep 18, 2019

Review usage of arrow.get() for Arrow 0.15.0 #313

Review usage of arrow.get() for Arrow 0.15.0 #313

Conversation

davidag commented Sep 10, 2019 • edited Loading

davidag commented Sep 11, 2019

k4nar left a comment

Choose a reason for hiding this comment

davidag commented Sep 11, 2019 • edited Loading

k4nar commented Sep 11, 2019

davidag commented Sep 11, 2019

jmaupetit commented Sep 11, 2019

davidag commented Sep 11, 2019

jmaupetit commented Sep 11, 2019

jmaupetit commented Sep 13, 2019

davidag commented Sep 13, 2019

ryanmjacobs commented Sep 13, 2019 via email

davidag commented Sep 16, 2019

ryanmjacobs commented Sep 17, 2019

ryanmjacobs commented Sep 18, 2019 • edited Loading

davidag commented Sep 18, 2019

ryanmjacobs commented Sep 18, 2019 via email

davidag commented Sep 18, 2019

davidag commented Sep 10, 2019 •

edited

Loading

davidag commented Sep 11, 2019 •

edited

Loading

ryanmjacobs commented Sep 18, 2019 •

edited

Loading