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

Remove RTD files and consolidate all requirements into one file #298

Merged
merged 18 commits into from
Apr 19, 2017

Conversation

cdeepakroy
Copy link
Contributor

This PR includes the following changes

  • Removes any files specific to ReadTheDocs as we are not using it to host documentation anymore.
  • Consolidates all the requirements (currently spread across multiple files) into one file and change setup.py to accomodate this.

@cdeepakroy cdeepakroy requested review from salamb and removed request for salamb April 18, 2017 17:05
@@ -31,8 +31,7 @@ installed as follows::
$ git clone https://github.com/DigitalSlideArchive/HistomicsTK.git
$ cd HistomicsTK
# pip install git+https://github.com/cdeepakroy/ctk-cli
$ pip install --no-cache-dir -r requirements_c_conda.txt
$ pip install --no-cache-dir -r requirements.txt -r requirements_c.txt
$ pip install --no-cache-dir -r requirements.txt

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to put the ctk-cli package in the requirements file as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is part of my plan!

I tried adding it, but parse_requirements was throwing an error. So i am first making sure TravisCI succeeds without adding it to the requirements.txt file to isolate the issue. Then i will add it back and check.

Copy link
Contributor Author

@cdeepakroy cdeepakroy Apr 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This piece of code in setup.py where we use pkg_resources.parse_requirements to read in all dependencies from the requirements.txt file fails if we have editable requirements like -e git+https://github.com/cdeepakroy/ctk-cli#egg=ctk_cli.

Look at this discussion on a pypa/pip github issue and this discussion on stackoverflow that complicates doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will refrain from adding it to requirements.txt in this PR as i don't have a solution for this issue at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctk_cli is anyway not needed when HistomicsTK is used as a pure python toolkit for analysis.

@codecov-io
Copy link

codecov-io commented Apr 18, 2017

Codecov Report

Merging #298 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #298   +/-   ##
=======================================
  Coverage   44.26%   44.26%           
=======================================
  Files           2        2           
  Lines          61       61           
=======================================
  Hits           27       27           
  Misses         34       34

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7e772...6c31d80. Read the comment docs.

@manthey
Copy link
Contributor

manthey commented Apr 18, 2017

Can you also modify the ansible/roles/girder/tasks/main.yml file? There are some references for when we aren't making docker files (or ping @kotfic and make sure he does it when he fixes the vagrant up path).

@cdeepakroy
Copy link
Contributor Author

@manthey Made a commit to fix references to requirements files in ansible roles. Can you please check if i got everything?

@kotfic This PR consolidates all the dependencies into a single requirements.txt file.

@cdeepakroy cdeepakroy requested a review from manthey April 18, 2017 20:55
@kotfic
Copy link
Contributor

kotfic commented Apr 19, 2017

I believe you have all the ansible references sorted, once this goes in @danlamanna will need to merge/rebase his refactor branch, that will propagate down to the vagrant fixes and we should be all set.

requirements.txt Outdated
# scientific packages
#
Cython>=0.24
nimfa==1.2.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the nimfa requirement to >=?

#
###############################################################
lxml>=3.4.4
docker-py==1.9.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need docker-py for the deploy-docker script. Perhaps it should be moved to requirements-dev.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should i also change everything in requirements_dev.txt to >=

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docker-py needs to be >=1.9,<2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, we probably just need to check to make sure they work with the current latest.

@cdeepakroy cdeepakroy merged commit d2356af into master Apr 19, 2017
@cdeepakroy cdeepakroy deleted the consolidate-requirements branch April 25, 2017 13:00
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.

5 participants