Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Dependancies and tests #26

Merged
merged 18 commits into from
Nov 28, 2017
Merged

Fix Dependancies and tests #26

merged 18 commits into from
Nov 28, 2017

Conversation

aabdullah-bos
Copy link
Contributor

This pull request adds pandas-datareader and seaborn to the dependency list in setup.py, since they are needed by the data module and the charting module.

When I ran tox I noticed that the integration test for the quickstart was failing because the interfaces for the Data Generator sub-classes had not been expanded to accept cahce_path and data_path values.

I'm not sure if you wanted to expose these as parameters to the API, but it seemed worthwhile. Anyway, this commit should fix any install problems as well as get the test to start passing.

Aquil Abdullah added 5 commits September 28, 2017 16:06
* Out of the box I was unable to run the quickstart.py
  example, because the setup.py did not require __pandas-datareader__
  or __seaborn__.
* It looks like there was a change to the the generator interface
  that was intended, but not implemented. When I attempted to run tox
  I encountered the error:

  E  TypeError: __init__() got an unexpected keyword argument 'cache_path'

  This was because the attempt to create an instance of the YahooData
  generator was passing cache_path, which was not in the parameter list
  for the class. I've added both cache_path and data_path as well as
  `__init__` methods for all generators so that they invoke the base
  class `__init__` method.
* Forgot to include this as part of the update.
Aquil Abdullah added 2 commits October 5, 2017 14:21
* The charting module improts seaborn, althought it doesn't use it.
  I am assuming that seaborn may be used in the future. Seaborn
  requires importlib, which is not part of the Python Standard
  Library in Python 2.6, so I am removing 2.6 from the travis
  file so that higher versions are installed.

  I could have gone with a conditional install, but it seemed like
  it was more trouble than it was worth. I could have also just
  removed the seaborn dependency, but since I am not the maintainer
  I will leave that decision to him. If the decision is made to
  remove the seaborn depencdency then support for Python 2.6 can
  be added back.
@Emsu
Copy link
Owner

Emsu commented Oct 9, 2017

@aabdullah-bos Looks good. 3.2 - 3.4 seem broken still. Is that something that's a quick fix on your end? If not happy to merge this in first since it cleans up tests for the most recent Python versions.

@aabdullah-bos
Copy link
Contributor Author

@Emsu Yeah, I saw that. I think that the issue only has to do with the test, and not the code under test, so it should be safe to merge. There is something different about how relative imports are handled in Python 3.2, 3.3, and 3.4. I'll look into it and submit a separate fix so that all the test work.

@aabdullah-bos
Copy link
Contributor Author

So, I tried running tox in a docker container for py3.2 and I got the following warning:

root@2fa1d7361b90:~/prophet# tox -e py32
/usr/local/lib/python3.2/site-packages/pkg_resources/__init__.py:87: UserWarning: Support for Python 3.0-3.2 has been dropped. Future versions will fail here.
  warnings.warn(msg)

I then got error messages associated with my docker container not having Python 2.7 installed. Anyway, I then ran a Docker container with Python3.4 installed and the test passed:

tox -e py34 -v
using tox.ini: /root/prophet/tox.ini
using tox-2.9.1 from /usr/local/lib/python3.4/site-packages/tox/__init__.py
py34 reusing: /root/prophet/.tox/py34
  /root/prophet$ /root/prophet/.tox/py34/bin/python /root/prophet/setup.py --name
  /root/prophet$ /root/prophet/.tox/py34/bin/python -c import sys; print(sys.path)
py34 develop-inst-noop: /root/prophet
  /root/prophet$ /root/prophet/.tox/py34/bin/pip freeze >/root/prophet/.tox/py34/log/py34-4.log
py34 installed: alabaster==0.6.1,argh==0.26.2,backports-abc==0.5,certifi==2017.7.27.1,chardet==3.0.4,cycler==0.10.0,docutils==0.14,idna==2.6,Jinja2==2.9.6,livereload==2.2.0,MarkupSafe==1.0,matplotlib==2.1.0,mock==1.0.1,numpy==1.13.3,pandas==0.15.1,pandas-datareader==0.5.0,pathtools==0.1.2,-e git+git@github.com:aabdullah-bos/prophet.git@e7b8fd05cf511c9f91158448d4478e1166e17d32#egg=prophet,py==1.4.34,Pygments==2.2.0,pyparsing==2.2.0,pytest==3.2.3,python-dateutil==2.6.1,pytz==2017.2,PyYAML==3.12,requests==2.18.4,requests-file==1.4.2,requests-ftp==0.3.1,scipy==0.19.1,seaborn==0.8.1,six==1.8.0,Sphinx==1.2.3,sphinx-autobuild==0.3.0,sphinxcontrib-napoleon==0.2.8,tornado==4.5.2,urllib3==1.22,watchdog==0.8.3
py34 runtests: PYTHONHASHSEED='3537394357'
py34 runtests: commands[0] | py.test tests
  /root/prophet$ /root/prophet/.tox/py34/bin/py.test tests
============================================================================================================================= test session starts ==============================================================================================================================
platform linux -- Python 3.4.7, pytest-3.2.3, py-1.4.34, pluggy-0.4.0
rootdir: /root/prophet, inifile:
collected 1 item

tests/integration/test_examples.py .

=========================================================================================================================== 1 passed in 6.67 seconds ===========================================================================================================================
___________________________________________________________________________________________________________________________________ summary ____________________________________________________________________________________________________________________________________
  py34: commands succeeded
  congratulations :)

So while, I'm still not sure the Python 3.4 test failed. You may want to consider dropping Python 3.2 and Python 3.3 from the test matrix, but that's a decision. I'll leave to you!

Aquil Abdullah added 11 commits October 11, 2017 21:45
* Since some python versions can't find prophet
  I am explictly setting the PYTHONPATH
* Added line to skip missing interpreters
Not needed.
* The test infrastructure itself shouldn't need
  to specify the pandas version.
* I encountered a number of different issues attempting
  to run the tests for Python 3.2 and 3.3.

  It looks like the docker image being used for 3.2
  is using pip 7.1.2 and has an issue with the syntax in
  install_requires section of the setup.

  It also looks like the testing of 3.3 has an issue with
  installing numpy. The error message ends with:

  ```
  RuntimeError: Python version 2.7 or >= 3.4 required.
  ```

  If others have concern you should investigate these issues
  further.
@aabdullah-bos
Copy link
Contributor Author

@Emsu I've made a few changes to both the travis and tox configurations. Below is a brief description of my rationale.

  1. I dropped testing support for Python 2.6, importlib is not part of the standard library in 2.6, but is needed by seaborn.
  2. I dropped testing support for Python 3.2, because there was an issue with pip understanding the install_requires.
  3. I dropped testing support for Python 3.3, because of dependency issues with the latest versions of pandas and numpy.
  4. I added support for Python 3.5, 3.6 which are later versions of python.
  5. I removed the sdist install in the travis file, because it was redundant since you set usedevelop in the tox configuration.
  6. I changed the command pytest to tox since that seemed like what you want.

NOTE: You should probably add a deploy step to the travis configuration to build the sdist and wheels after the test run successfully for tag builds. I can help with that once you've got this commit merged.

@aabdullah-bos
Copy link
Contributor Author

@Emsu Any thoughts on this pull request?

@Emsu
Copy link
Owner

Emsu commented Nov 28, 2017

@aabdullah-bos looks good. Sorry for the delay.

@Emsu Emsu merged commit 8d1ec1c into Emsu:master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants