search

zipline-live / zipline

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

remove IbPy requirements from CI #23

Closed bartosh closed 7 years ago

bartosh commented 7 years ago

Removed requiements_live_ib from Travis and Appveyor configs to decouple core from broker-specific requirements.

Signed-off-by: Ed Bartosh bartosh@gmail.com

tibkiss commented 7 years ago

The reason it fails is explained here: Nose introspects all the module and tries to import it. As broker is a module it ends up importing ib broker. More details could be found here: https://stackoverflow.com/questions/3073259/python-nose-import-error

The reason AppVeyor passed in #12 is because init.py was removed from gens/brokers/. I believe that's a mistake, brokers is indeed a module, it needs init.py

Removing IbPy from the CI build is just a workaround to support AppVeyor. I'd rather drop AppVeyor support than remove IbPy installation from CI.

Because of this I'd rather not pull this change in: -1

bartosh commented 7 years ago

@tibkiss > The reason AppVeyor passed in #12 is because init.py was removed from gens/brokers/

It was not removed. It was made empty.

Anyway, I think it makes sense to fix this or at least work around it. Otherwise we'll end up changing upstream files for no need and installing modules that are not used in our tests.

bartosh commented 7 years ago

Just looked at #12 again. you're right - brokers/init.py was removed. I didn't plan to do that though. I wanted to make it empty. For some reason git decided to remove it.

bartosh commented 7 years ago

There should be a way to tell nose not to try importing everything. That doesn't look right to me. I thought nose only cares about test.py files or test/ directories. Apparently it was wrong assumption.

pbharrin commented 7 years ago

I don't see any harm in having the IB module imported in CI. In fact I don't see any problems with importing modules from random brokers that are supported, they are just imported for the CI tools. It is not forcing users to install these modules. Now the builds are breaking.

I'd also rather not pull in this change.

bartosh commented 7 years ago

well, I mentioned potential problems - loading modules without any intent to use them and changing upstream files just for that.

I'm not asking you to merge this until I fix both tests. I'm generally against merging any PR that breaks tests.

tibkiss commented 7 years ago

IbPy is a real requirement, which needs to be installed if one uses IB as a broker. Similarly, you can find 'blaze' package installed on AppVeyor which is not part of the normal installation.

I believe the only reasonable change is to fix the Appveyor IbPy installation problem, then the rest of the testcases will pass.

bartosh commented 7 years ago

IbPy is a real runtime requirement, not a test requirement. blaze is a test requirement as it's used in tests/pipeline/test_blaze.py. The fact that nose tries to import everything is not a reason to install something that is not used in tests when we run tests.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 86.054% when pulling 87bea6260e656677857d44f73784844e208692d6 on ed into d0be2fc458472b241ffaf1dfee15b8bcdbc105f1 on live.

tibkiss commented 7 years ago

Sorry, but I don't agree with this change.

CI should be able to install our software without any nose-trick. If AppVeyor is not able to kick off the tests then it is likely that our code does not work on windows platform.

bartosh commented 7 years ago

Sorry, I'm not sure I fully understand your statement. How installation of the software is related to nose? My last change stops nose from loading module that's not related to tests. It doesn't do anything with installation I think. Please elaborate.

tibkiss commented 7 years ago

By 'install our software' I meant dependencies.

My point is that this is an unorthodox workaround to make the build green, even though we know the IB Broker will not function under Windows due to lack of IbPy2 package. CI tools should be able to install our dependencies if we are supporting that platform.

bartosh commented 7 years ago

If we ever planning to merge our code upstream (we should in my opinion) we shouldn't make tests depending on the packages that are not used in tests and are optional. I consider broker interfaces as an optional, plugin-like additions that only make sense when particular broker is used. Installing everything that all possible broker interfaces depend on doesn't make much sense and definitely will not be accepted upstream.

tibkiss commented 7 years ago

"Installing everything that all possible broker interfaces depend on doesn't make much sense and definitely will not be accepted upstream." I beg to differ on 'install everything' approach, please take a look here: https://github.com/zipline-live/zipline/blob/master/.travis.yml#L55-L60 Also, I don't think we'll ever be merged upstream.

Having ib_broker implemented during test has benefits: it ensures that there are no trivial mistakes made on that file.

What do you think about adding a proper win64 based installation to conda? It does not seem like a big effort and would resolve our problem cleanly.

bartosh commented 7 years ago

I'm still not convinced. Not sure why everything is installed in zipline .travis.yml. May be they have a good reason for that. Even if they don't we should do the same. I'd understand if we install ib because we have tests that use it. Otherwise it's not good to require it without actual reason.

What do you think about adding a proper win64 based installation to conda? It does not seem like a big effort and would resolve our problem cleanly.

Which problem?

tibkiss commented 7 years ago

"I'm still not convinced." That makes the three of us. Neither Peter nor me agrees to your change.

"Which problem?" AppVeyor builds are failing.

Let us know when you are convinced.

bartosh commented 7 years ago

That makes the three of us. Neither Peter nor me agrees to your change.

Yep, I see.

AppVeyor builds are failing.

can you be a bit more specific, please? You mentioned AppVeyor "problem" and failing builds several times, but never pointed out to any evidence:

Here are the latest builds and they're green:

tibkiss commented 7 years ago

I'm sorry, I did not knew that the AppVeyor build has been fixed by adding pip install. Previously conda was unable to install this requirement: https://ci.appveyor.com/project/pbharrin/zipline/build/1.0.31/job/us969yt5yivd5hqh

Before I read your comment I made IbPy2 available in Conda for all platforms: https://anaconda.org/tibkiss/ibpy2