Use UTC datetimes everywhere #69

rubik · 2016-04-09T10:47:40Z

What changed

All datetime math is now done in UTC. DateTimeFields are guaranteed to be already UTC since when USE_TZ = True Django will set PostgreSQL's timezone config option to UTC, as explained here:
https://docs.djangoproject.com/en/1.9/ref/databases/#optimizing-postgresql-s-configuration;
created_on and modified_on do not rely anymore on get_time, everything is done automatically with auto_now=True and auto_now_add=True. Those two options use internally timezone.now();
a new migration has been generated to alter the aforementioned fields;
~~created_on_str is now saved in UTC. In this way the DB is not tied to a specific timezone. However, it's still not perfect, since the old data is still in MST and a migration could be necessary.~~ ~~created_on_str remained in MST.~~
Thanks to django-chartit2, created_on_str and date_str have been removed.

Migration

Since PostgreSQL stores all the datetimes as UTC, no DB migration is required. However, some Django fields have been altered, so a python manage.py migrate is necessary to get things working.

Notes

created_on and modified_on are no longer shown in the admin. This is due to the fact that those two attributes are now managed automatically by Django.

For reference, see #9.

owocki · 2016-04-10T19:15:56Z

👏 thanks!

i want to

verify theres no custom datetime mutation logic that this PR missed
test locally

before merging.

rubik · 2016-04-10T20:14:14Z

Sure, let me know if anything goes wrong.

rubik · 2016-04-11T12:38:37Z

Maybe I can open a PR with the linting, get it merged and then rebase my branch on master so that only the relevant changes are left in this PR? That would make it easier to review.

owocki · 2016-04-11T16:36:45Z

I still see a little bit of timezone mutation on this branch

Searching 71 files for "hours=7"

/Users/kevinowocki/pytrader/history/views.py:
  555      data = {}
  556      for t in Trade.objects.filter(symbol=symbol,status='fill').order_by('-created_on').all():
  557:         date = datetime.datetime.strftime(t.created_on-datetime.timedelta(hours=7),'%Y-%m-%d')
  558          if not date in data.keys():
  559              data[date] = { 'buyvol' : [], 'sellvol' : [], 'buy' : [], 'sell': [], 'bal' : 0.00 }

/Users/kevinowocki/pytrader/history/management/commands/compare_perf.py:
   56                               directionally_same=directionally_same,
   57                               directionally_same_int=1 if directionally_same else 0,
   58:                              created_on_str=(tr_timerange_end - datetime.timedelta(hours=7)).strftime('%Y-%m-%d %H:%M'))
   59          pc.save()
   60  

2 matches across 2 files

owocki · 2016-04-11T16:38:58Z

While I agree that storage in UTC is the right way to go, I'd really like to be able to display data in my local timezone. This prevents me from having to do timezone math in my head everytime I look at some data. Any thoughts on the best way to do that?

owocki · 2016-04-11T16:39:13Z

@rubik merged your previous PR.

owocki · 2016-04-11T16:40:40Z

However, it's still not perfect, since the old data is still in MST and a migration could be necessary.

Is it worth writing a data migration for this? I know there's 5-10 others from slack that are running the repo, so I know it's not just me that has old data in the DB.

rubik · 2016-04-11T18:41:46Z

Is it worth writing a data migration for this? I know there's 5-10 others from slack that are running the repo, so I know it's not just me that has old data in the DB.

Fair enough, I don't think there's a good solution to this. I'll just revert the created_on_str changes.
I also think that this is temporary. Either #35 comes up with a good solution (which maybe does not use django-chartit), or we'll look into some alternatives (perhaps for performance reasons).
The problem is that django-chartit is not really maintained anymore, so the issue relevant to us (chartit/django-chartit#2) is never going to be fixed.

rubik · 2016-04-14T11:04:07Z

history/management/commands/pull_balance.py

-        for b in Balance.objects.filter(date_str='0'):
-            # django timezone stuff , FML
-            b.date_str = datetime.datetime.strftime(b.created_on - datetime.timedelta(hours=int(7)),'%Y-%m-%d %H:%M')
+        for b in Balance.objects.filter(date_str=''):


Is date_str='' correct or was it correct before?

yes you are correct https://github.com/owocki/pytrader/blob/master/history/models.py#L207

rubik · 2016-04-14T11:05:23Z

I suggest you test locally, since now I'm not able to. If everything works, I think it's simpler to merge this one after #72.

owocki · 2016-04-14T13:49:11Z

this will need a rebase before merging.

rubik · 2016-04-14T15:48:36Z

history/views.py

    for t in Trade.objects.filter(symbol=symbol, status='fill').order_by('-created_on').all():
-        date = datetime.datetime.strftime(t.created_on-datetime.timedelta(hours=7), '%Y-%m-%d')
+        mst_dt = timezone.localtime(t.created_on, pytz.timezone('MST'))
+        date = datetime.datetime.strftime(mst_dt, '%Y-%m-%d')


I had forgotten this one. It should be MST since it's in a view.

MST was just what i used because thats where i live. might be worth making this a settings.DISPLAY_TIMEONE for others to configure.

But then DBs would be inconsistent between each other.

Maybe it's best to leave it MST and look for a django-chartit alternative.

But then DBs would be inconsistent between each other.

could be solved with a migration

Maybe it's best to leave it MST and look for a django-chartit alternative.

i dont feel strongly about this enoguh to hold up the PR. just a suggestion

could be solved with a migration

Well in that case we would have to store the data in UTC, but at that point it would be useless. We could remove created_on_str and add an attribute on the fly. Or use a Python property. Is it possible? Currently I don't have data to create a chart and check myself. I only have the seed from #2.

IIRC, django-chartit requires the data to be available within the queryset, so a python property will likely not work.

data is posted @ #2 (comment) not asking that you make the change, just telling you about this because now that you've made a PR you should at least have access to the data so you can trade :)

Can this line be:

dt = timezone.localtime(t.created_on, pytz.timezone(settings.DISPLAY_TZ))

?

Yes I think it should be

rubik · 2016-04-14T15:49:04Z

There you go. It should be good now.

rubik · 2016-04-15T13:00:11Z

history/views.py

+        trades = Trade.objects.filter(
            symbol=symbol, created_on__gte=start_time).filter(status__in=['fill', 'open', 'error']).order_by('id')
    else:
-        trades = Trade.objects.exclude(created_on_str="").filter(


See comment below about created_on_str=''.

rubik · 2016-04-15T13:05:53Z

From my experiments, 'categories': 'created_on' works fine. The real bottleneck was 'terms': 'created_on' which didn't work in django-chartit. With django-chartit2 it's possible to specify a function to handle the data. I wasn't able to test the code with the real data because I only have the prices (I never traded).

This PR needs #74.

owocki · 2016-04-18T13:37:06Z

I'm good with merge here. @igorpejic @Snipa22 @t0mk ?

igorpejic · 2016-04-18T18:18:16Z

history/management/commands/pull_deposits.py

            d.type = 'deposit' if amount > 0 else 'withdrawal'
            d.status = status
            d.created_on = created_on
            d.modified_on = created_on


This line has no effect since, auto_now_add is used for modified_on?

Good catch! We are in the situation described here:
#75 (comment)

I think @owocki intention was to capture the time of creation on the poloniex server with this.
A proposal could be we add deposit_created_on to Deposit model and use created_on for the creation of the model in our database.
In short:
deposit_created_on - time of creation at poloniex server
created_on - time of saving in our database

Seems reasonable. The only downside of this approach is that we will have to remember to use deposit_created_on instead of created_on.

owocki · 2016-04-23T14:36:06Z

would like to get another to sign off on merge here... @Snipa22 @igorpejic @t0mk ?

owocki · 2016-04-23T14:37:33Z

@rubik could you describe how you tested this? does your personal instance of pytrader have data flowing through the admin?

rubik · 2016-04-24T07:26:00Z

@owocki I had price data in the admin and the chart was displaying correctly. Then I generated fake data in another Django project to test PivotChart and it was working correctly.
If you could test it with trade data it would be even better. Locally I an only run predict_many_sk.py and it appears that c_chart works as intended.

What do you think about adding deposit_created_on? (See above.)

rubik mentioned this pull request Apr 11, 2016

Remove trailing spaces #70

Merged

rubik force-pushed the all-utc branch from 183fdfe to bc5494b Compare April 11, 2016 18:19

rubik reviewed Apr 14, 2016
View reviewed changes

rubik force-pushed the all-utc branch from 8086081 to ce4e8dc Compare April 14, 2016 15:41

Use UTC datetimes everywhere

e4b9a88

rubik force-pushed the all-utc branch from 97daf16 to e4b9a88 Compare April 14, 2016 15:47

rubik reviewed Apr 14, 2016
View reviewed changes

rubik reviewed Apr 15, 2016
View reviewed changes

Remove date_str and created_on_str from the models and the views

8ecab65

rubik force-pushed the all-utc branch from 6db44c8 to 8ecab65 Compare April 15, 2016 14:06

igorpejic reviewed Apr 18, 2016
View reviewed changes

Replace nonexistent date with created_on

e6d087a

rubik force-pushed the all-utc branch from b0b4615 to e6d087a Compare April 18, 2016 19:43

Uh oh!

Use UTC datetimes everywhere #69

Are you sure you want to change the base?

Use UTC datetimes everywhere #69

Uh oh!

Conversation

rubik commented Apr 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Migration

Notes

Uh oh!

owocki commented Apr 10, 2016

Uh oh!

rubik commented Apr 10, 2016

Uh oh!

rubik commented Apr 11, 2016

Uh oh!

owocki commented Apr 11, 2016

Uh oh!

owocki commented Apr 11, 2016

Uh oh!

owocki commented Apr 11, 2016

Uh oh!

owocki commented Apr 11, 2016

Uh oh!

rubik commented Apr 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubik commented Apr 14, 2016

Uh oh!

owocki commented Apr 14, 2016

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!

rubik Apr 14, 2016 • 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!

igorpejic Apr 18, 2016 • 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!

rubik commented Apr 14, 2016

Uh oh!

rubik Apr 15, 2016 • 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!

rubik commented Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owocki commented Apr 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubik Apr 18, 2016 • 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!

owocki commented Apr 23, 2016

rubik commented Apr 9, 2016 •

edited

Loading

rubik Apr 14, 2016 •

edited

Loading

igorpejic Apr 18, 2016 •

edited

Loading

rubik Apr 15, 2016 •

edited

Loading

rubik commented Apr 15, 2016 •

edited

Loading

rubik Apr 18, 2016 •

edited

Loading

rubik commented Apr 24, 2016 •

edited

Loading