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 pip install option from TARDIS installation docs #1300

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

jaladh-singhal
Copy link
Member

@jaladh-singhal jaladh-singhal commented Sep 14, 2020

pip install git+https://github.com/tardis-sn/tardis seems not to be working as mentioned in #1211 and as Anaconda suggests that pip should better be avoided with conda, so it needs to be removed from TARDIS installation docs to avoid any confusion for new users.

The only recommended way in our case is to use python setup.py install which is already mentioned in docs.

Motivation and Context

Avoid pip in conda and pip install not working but is still mentioned in docs

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have assigned/requested two reviewers for this pull request.

@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1300   +/-   ##
=======================================
  Coverage   80.54%   80.54%           
=======================================
  Files          41       41           
  Lines        3423     3423           
=======================================
  Hits         2757     2757           
  Misses        666      666           

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 862025e...c738838. Read the comment docs.

@epassaro
Copy link
Member

I think the Installation guide now looks a bit redundant. If you are cloning the repo at some point then you don't need to get the environment file through curl or wget:

  1. Clone the TARDIS repository
  2. Install the dependencies with the YAML file in the repository
  3. python setup.py install

Then, I don't think this closes #1211. Anaconda discourages the use of pip and that's fine. But it was possible to install TARDIS through pip and we break that at some point (and we don't know exactly why) so the bug still exists.

@epassaro
Copy link
Member

I think the Installation guide now looks a bit redundant. If you are cloning the repo at some point then you don't need to get the environment file through curl or wget:

  1. Clone the TARDIS repository
  2. Install the dependencies with the YAML file in the repository
  3. python setup.py install

Then, I don't think this closes #1211. Anaconda discourages the use of pip and that's fine. But it was possible to install TARDIS through pip and we break that at some point (and we don't know exactly why) so the bug still exists.

what do you think about this @jaladh-singhal ?

@wkerzendorf
Copy link
Member

@jaladh-singhal the tests fail

@jaladh-singhal
Copy link
Member Author

@wkerzendorf It's only for Mac pipeline due to that unrelated test

@jaladh-singhal
Copy link
Member Author

Then, I don't think this closes #1211. Anaconda discourages the use of pip and that's fine. But it was possible to install TARDIS through pip and we break that at some point (and we don't know exactly why) so the bug still exists.

@epassaro this makes sense, I agree it shouldn't closed #1211 - I've removed that from description

I think the Installation guide now looks a bit redundant. If you are cloning the repo at some point then you don't need to get the environment file through curl or wget:

This is true @epassaro but we still need to make sure that pip install isn't recommended until we fix it. So either I can change the wording to "Clone TARDIS Repo -> Create env (without curl/wget ofc) -> setup.py install. Or the other way is we leave it as it is and mark it "not working currently" or something and put setup.py install section (without curl/wget) before it, marking as "recommended way as of now". Let me know which one do you prefer?

@epassaro
Copy link
Member

Then, I don't think this closes #1211. Anaconda discourages the use of pip and that's fine. But it was possible to install TARDIS through pip and we break that at some point (and we don't know exactly why) so the bug still exists.

@epassaro this makes sense, I agree it shouldn't closed #1211 - I've removed that from description

I think the Installation guide now looks a bit redundant. If you are cloning the repo at some point then you don't need to get the environment file through curl or wget:

This is true @epassaro but we still need to make sure that pip install isn't recommended until we fix it. So either I can change the wording to "Clone TARDIS Repo -> Create env (without curl/wget ofc) -> setup.py install. Or the other way is we leave it as it is and mark it "not working currently" or something and put setup.py install section (without curl/wget) before it, marking as "recommended way as of now". Let me know which one do you prefer?

I prefer the first option.

@wkerzendorf wkerzendorf merged commit 56b3a9d into master Nov 4, 2020
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants