zipline-live / zipline

Zipline-Live, a Pythonic Algorithmic Trading Library
http://www.zipline-live.io/
Apache License 2.0
394 stars 65 forks source link

Merge latest changes from quantopian's zipline/master #83

Closed amarin15 closed 4 years ago

amarin15 commented 6 years ago

The master branch is currently 113 commits ahead, 263 commits behind quantopian:master. This merges the missing 263 commits from quantopian's master. The only files that I had to fix conflicts for were:

So when reviewing, you only really need to look at the changes in the files above and see if they make sense. I kept the zipline-live version of README.rst and quantopian version of example_data.tar.gz and SPY_benchmark.csv. If you want to see the merge commit, the GitHub UI displays all the changes it pulls in, but git show 43a3bc outputs only the merge conflicts that were fixed: https://gist.github.com/alexukf/6cd8a7e031b2b78a331a9768b40f200c

tibkiss commented 6 years ago

Thanks for this contribution.

This is a moderately risky change given the amount of code change we introduce. Did you run this code live with an algorithm? Normally I run my changes for one week in a Canary setup, before I push it into the repo. I'd appreciate if you could do the same here.

I'm wondering why we have the godzilla 'Rebase on top of quantopian/master' commit, if it is a rebase?

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 86.293% when pulling 1251a45737f6327a0700b57d26d354fd4f7f130b on alexukf:rebase into 825f643a14152e721a06eb5bf9a56e2249431a41 on zipline-live:master.

tibkiss commented 6 years ago

One more thing: to aid reviewing, could you please put your changes in the conflicting files into separate commits? Doing so might also remove the 'Rebase on top of quantopian/master' all-encompassing commit. Thanks!

amarin15 commented 6 years ago

I merged all the latest changes from quantopian in a single commit. Unfortunately it's not possible to put the changes in the files with conflicts into separate commits (at least not trivially), since you can't commit with unresolved conflicts while doing a git merge. If you want to see the merge commit, the GitHub UI does indeed display all the changes it pulls in, but git show 43a3bc locally outputs only the merge conflicts that were fixed: https://gist.github.com/alexukf/6cd8a7e031b2b78a331a9768b40f200c An alternative would be to only look at the 8 (5 really) files that had conflicts in the PR (see new PR description).

Tested with nosetests locally, but did not test with a live algo yet. I did manually go through git blame for all the conflicting files to see which are the most recent changes, but as you mentioned, this should probably be merged after it's tested with a live algo.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.02%) to 86.456% when pulling 43a3bcee3f2cb9d7ef219a0d7d50693e28d2f461 on alexukf:rebase into 825f643a14152e721a06eb5bf9a56e2249431a41 on zipline-live:master.

tibkiss commented 6 years ago

Thanks @alexukf , I'll try to review it next week.

tibkiss commented 6 years ago

@alexukf : Thanks for this work. I'm afraid there is a misunderstanding here.

To achieve maintainable fork we need to avoid having large merge commits, such as the 'Merge latest quantopian changes' which encompasses 200+ changes into a single change.

I'd suggest that you take https://github.com/zipline-live/zipline/pull/71 as an example, where the original author, commit message is retained.

Additionally, I'm just about to post a PR for https://github.com/zipline-live/zipline/issues/84 which will cause merge conflicts. I'd prefer to have that PR merged first for two reasons: a) it brings a new feature b) it was extensively tested (using in production for two weeks now).

I'm happy to help executing and testing the rebase, but I'd recommend we sync up in chat to reduce the chance of further mis-communication. Thanks for your understanding.

tibkiss commented 4 years ago

This change became outdated. Closing.