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

Restrict ipykernel, tornado and pyzmq versions so do not get extra figs when wiggle ipywidgets #210

Merged
merged 7 commits into from
Nov 28, 2018

Conversation

willfurnass
Copy link
Collaborator

@willfurnass willfurnass commented Oct 24, 2018

Hopefully fixes Issue #158 i.e. ensure that superfluous figures aren't created when wiggling ipywidgets after installing just the MuMoT package (i.e. using just the requirements specified in setup.py, not using a requirements.txt or environment.yml). #100 can therefore also be closed if this works.

Some explanation:

  • superfluous widgets are created by MuMoT unless the ipykernel package is < 4.7.
  • Using ipykernel < 4.7 requires that the tornado package is < 5 (otherwise you get RuntimeError: This event loop is already running)
  • Using tornado < 5 requires that the pyzmq package is < 17 (otherwise you get ValueError: signal only works in main thread)
  • Using pyzmq package is < 17 requires that the notebook package is <5.5 (otherwise you see the following warning at install time: notebook 5.7.0 has requirement pyzmq>=17, but you'll have pyzmq 16.0.4 which is incompatible.)

@jarmarshall @tbose1 @joefresna Please test locally on macOS and Windows in Python 3.6 virtualenvs and/or conda envs.

If this works we need to:

  • Remove conda env definitions
  • Add minimal requirements.txt for binder (just '.' i.e the package in the current directory)
  • Update binder config
  • Test binder
  • Update install instructions in docs

@willfurnass willfurnass added this to the First release milestone Oct 24, 2018
@willfurnass willfurnass self-assigned this Oct 24, 2018
@codecov-io
Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #210 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
- Coverage   58.82%   58.79%   -0.04%     
==========================================
  Files           6        6              
  Lines        8377     8377              
  Branches     1769     1769              
==========================================
- Hits         4928     4925       -3     
- Misses       2986     2988       +2     
- Partials      463      464       +1
Impacted Files Coverage Δ
mumot/__init__.py 66.84% <0%> (-0.06%) ⬇️

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 1cc9f1d...8fcb40d. Read the comment docs.

@jarmarshall
Copy link
Contributor

jarmarshall commented Oct 30, 2018

I am ready to review this but need to check how to do so - I've been using condo envs so imagine I have to deactivate my environment, then python -m pip install <path>?

@jarmarshall
Copy link
Contributor

That seems to work using the above method - however not sure how to include graphviz in the requirements using this installation method.
We need someone to look at this on Windows - maybe @renapagli - or we could ask @ajc158 to help when he's back

@willfurnass
Copy link
Collaborator Author

@jarmarshall You can install MuMot using pip an in a 'bare' Python 3.6 conda environment or in a Python 3.6 virtualenv. You can install graphviz using conda or using the OS pkg manager.

@jarmarshall
Copy link
Contributor

OK, so we will need to add graphviz installation to the revised installation instructions

I do worry slightly about how future-proof going down this route of restricting package versions is, but we already had to do that unfortunately to to dependencies on, e.g. PyDStool

@willfurnass
Copy link
Collaborator Author

OK, so we will need to add graphviz installation to the revised installation instructions

@jarmarshall graphviz is already listed in the install instructions: https://mumot.readthedocs.io/en/latest/install.html

I do worry slightly about how future-proof going down this route of restricting package versions is, but we already had to do that unfortunately to to dependencies on, e.g. PyDStool

There is far less package version pinning in setup.py than the conda environment definition files: only explicit requirements are listed in setup.py, whereas the conda environment definitions are exhaustive lists of the specific versions of all packages that were implicit and explicitly installed in an environment at a particular moment in time.

@jarmarshall
Copy link
Contributor

jarmarshall commented Nov 13, 2018

@joefresna does commit 83efeed mean we can close this? Have we verified that the duplicate figures issue is fixed on Windows by this? If so can @willfurnass resolve conflicts and tick off the remaining items to do?

Copy link
Contributor

@jarmarshall jarmarshall left a comment

Choose a reason for hiding this comment

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

Based on testing on Windows reported by @eliseofe this can be merged

@willfurnass
Copy link
Collaborator Author

The associated update to the docs (see checklist above) is in PR #229

@willfurnass
Copy link
Collaborator Author

Tested on mybinder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants