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

Replace pip install with conda install in Binder #54

Merged
merged 3 commits into from
May 17, 2020
Merged

Replace pip install with conda install in Binder #54

merged 3 commits into from
May 17, 2020

Conversation

davidbrochart
Copy link
Member

No description provided.

@choldgraf
Copy link
Contributor

Any particular reason for this? As a general rule my opinion is that it's better to use pip wherever possible

@davidbrochart
Copy link
Member Author

pip doesn't have a solver. If you look at the binder right now, it's broken because nbclient needs jupyter_client>=6 and something (maybe the Jupyter Notebook?) was installed after nbclient and installed jupyter_client==5.

@MSeal
Copy link
Contributor

MSeal commented May 15, 2020

I agree with @choldgraf on this one. I'm not sure quite what is meant by "pip doesn't have a solver"? Had the same binder issue with conda for an earlier build. The issue is more on the dependencies not getting rebuilt by the image build being served. Easier to just set the binder requirements to >= when necessary imo.

@davidbrochart
Copy link
Member Author

davidbrochart commented May 15, 2020

Maybe this particular issue is not caused by that, but what I mean by pip not having a solver is that if you install package A which needs C>=n and then package B which needs C<n, you will end up with C<n which will break A. Instead of having a broken environment, conda will try and find a solution that satisfies everybody. This is not a good example since the environment is unsolvable, but this is generally what a solver will do.
That being said, SAT solving is in the pipe for pip (pypa/pip#988), but IMHO conda is a better solution for package management anyway, not to mention python extensions and non python packages.

@choldgraf
Copy link
Contributor

I guess we disagree on this one - I usually tell people "try to do something with pip and only use conda if it isn't possible", pip is a python standard while conda is useful for a sub-community, and I think in general we should prefer standards over community-specific tools. That said, I recognize all of the benefits you just mentioned about conda, it does more reliably solve environments (even if it does take like 2 hours to complete 😅). This isn't a hill I will die on though, so if you really think conda will be better for this repository consider me a +0 on it.

@MSeal
Copy link
Contributor

MSeal commented May 16, 2020

I have the same opinion here as @choldgraf as I said. But I'm also fine with changing if you'd prefer it the other way. So I think we'll leave this to you to decide what you'd prefer here. aka Feel free to merge if you feel your preference is stronger than our milder inclination.

@davidbrochart
Copy link
Member Author

I could also live with using pip for our Binder, but the thing is that it is broken right now, so how can we fix that without switching to conda?

@choldgraf
Copy link
Contributor

@davidbrochart can you confirm that the Binder works for this branch? If so, I am happy to just merge this in since this is kind of "under-the-hood" changes anyway (it's not user-facing docs or anything)

@davidbrochart
Copy link
Member Author

@choldgraf
Copy link
Contributor

Sounds good - let's just go ahead and merge this one in, then. Thanks for taking the time to notice this, and also put in the fix @davidbrochart :-)

@choldgraf choldgraf merged commit e5d7528 into jupyter:master May 17, 2020
@davidbrochart
Copy link
Member Author

Thanks @choldgraf!

@davidbrochart davidbrochart deleted the conda branch May 17, 2020 09:32
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.

3 participants