GriffinAustin / pynance

Lightweight Python library for assembling and analysing financial data
MIT License
317 stars 43 forks source link

Updating dependencies. Adding an integration test. #15

Closed arkadasgupta closed 8 years ago

arkadasgupta commented 8 years ago

Updating dependencies to latest versions. Adding an integration test for data retrieval.

arkadasgupta commented 8 years ago

review comments addressed

aisthesis commented 8 years ago

I'm also noticing that I have test dependencies in my virtual env. I'm thinking we should probably create test requirements as well: http://stackoverflow.com/questions/4734292/specifying-where-to-install-tests-require-dependencies-of-a-distribute-setupto

Here's my requirements.txt:

beautifulsoup4==4.3.2 html5lib==0.999 lxml==3.4.2 matplotlib==1.4.3 nose==1.3.4 numpy==1.9.2 pandas==0.16.0 pandas-datareader==0.1.0 pyparsing==2.0.3 python-dateutil==2.4.1 pytz==2015.2 six==1.9.0

I know nose and pytz are only required for testing.

arkadasgupta commented 8 years ago

i think i didn't need the test dependencies because pycharm probably has them in-built. good idea. but i thought we were not 'fixing' the versions for now?

aisthesis commented 8 years ago

we're not freezing the versions. i'm just noting what the latest ones are for the record.

aisthesis commented 8 years ago

I also just updated my python 2 to 2.7.11 and python 3 to 3.5.1 and am setting up a virtual environment for each. Here's what I have before installing pynance itself:

cycler==0.9.0 lxml==3.5.0 matplotlib==1.5.1 numpy==1.10.4 pandas==0.17.1 pandas-datareader==0.2.1 pyparsing==2.0.7 python-dateutil==2.4.2 pytz==2015.7 requests==2.9.1 requests-file==1.4 six==1.10.0

Several of these are transitive dependencies and shouldn't need to appear in setup.py

aisthesis commented 8 years ago

We should be able to remove the following from the hard dependencies, because we don't depend on them directly: There's no file in pynance with an import foo statement for these packages that are in my requirements.txt after installing numpy, pandas, pandas-datareader and matplotlib:

lxml pyparsing python-dateutil pytz requests requests-file six

We do need:

matplotlib numpy pandas pandas-datareader

And as far as I can tell after grepping the repo, that's it. I think we should remove the following from the dependencies declaration:

lxml html5lib beautifulsoup4

Those are just transitive dependencies.

aisthesis commented 8 years ago

One other thing we should think about: It looks to me like the integration test will run along with all of the other unit tests when you type nosetests (or nosetests --exe). My intuition is that we should distinguish tests that can be run autonomously from tests that require connecting to external sources for data retrieval.

What's your opinion in that regard?

aisthesis commented 8 years ago

More on test dependencies: http://stackoverflow.com/questions/9607565/how-do-i-force-setup-py-test-to-install-dependencies-into-my-virtualenv

aisthesis commented 8 years ago

I think we should add in setup.py:

DEPENDENCIES_TST = [
    nose,
    pytz
    ]

and then in setup

tests_require=DEPENDENCIES_TST
arkadasgupta commented 8 years ago

I will make the changes to the setup.py and send it out again. As for the integration test, I was also kind of uncertain how to proceed. The best approach appeared to be having them as a separate package with a dependency on pynance. We don't publish this package to pip though. Wasn't sure of the conventions though. What do you think?

aisthesis commented 8 years ago

Not sure. Another option would be to put them in a separate subdirectory (same level as tst) called something like tst-integ. Let's also talk to some other engineers to get some additional opinions.

arkadasgupta commented 8 years ago

I don't know any experienced python devs aside from you. I'll see if I can find something on the net

arkadasgupta commented 8 years ago

I say let's not get blocked on this. I like the tst-integ approach for now. So let's just do it that way and get it checked in? We can always come back and fix things if required

aisthesis commented 8 years ago

http://nose.readthedocs.org/en/latest/usage.html

or, if necessary: https://pypi.python.org/pypi/nose-exclude

We could just put the test in an excluded directory with scripts to show how to run the different types of test.

Can we wait one more day for a final decision? I feel like it's important to set this up the right way.

arkadasgupta commented 8 years ago

Sure. I just wanted to get it done by Friday because of my limited availability after that.

aisthesis commented 8 years ago

I think we should do this:

That way, you can run all the tests using nosetests from root, or you can run a selection just by going to the appropriate directory.

aisthesis commented 8 years ago

Great work, Arka! Thanks.

Sorry to be so picky about various details on a pretty small change, but the directory structure and the listings will set a precedent for future releases, so I think it's important to have them rock solid.

arkadasgupta commented 8 years ago

I totally agree with the sentiments. I am equally picky about these things.