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

Updated to 5.0.0 #3

Merged
merged 11 commits into from
May 5, 2016
Merged

Updated to 5.0.0 #3

merged 11 commits into from
May 5, 2016

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Apr 20, 2016

Changed from PyPI to GitHub and using sha256 instead of md5.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@pelson
Copy link
Member

pelson commented Apr 20, 2016

Before merging this, it would be helpful to get some oversight from the @jdfreder. Is there an additional dependency which we should be pulling in (e.g. was the JS pulled out of this package and into widgetsnbextension).

@jdfreder
Copy link

jdfreder commented Apr 22, 2016

@pelson yeah, part of the Javascript was published to npm. The widgetsnbextension package installs that part and adds to it. The built (combined) static assets are a resource of the widgetsnbextension package (you can see this is the contents uploaded to pypi). Installing ipywidgets should automatically install widgetsnbextension. In order for the widgets to work in the notebook (only compatible with the newly released 4.2), jupyter nbextension enable --py widgetnbextension must be ran. cc. @SylvainCorlay

@jakirkham
Copy link
Member

jakirkham commented Apr 26, 2016

So, let me try to reduce this. Please correct me if I'm wrong.

  1. PyPI has all the Javascript code included.
  2. No new dependencies are required.
  3. Installation requires a second step (jupyter nbextension enable --py widgetnbextension).

@jdfreder
Copy link

Yes to 1 and 3, no to 2. ipywidgets is required as it was before and additionally widgetsnbextension is required.

@jakirkham
Copy link
Member

Hmm...I thought that widgetsnbextension was already included, no?

@jakirkham
Copy link
Member

FWIW, AppVeyor already passed, but the status didn't update for some reason.

@jdfreder
Copy link

jdfreder commented Apr 26, 2016

widgetsnbextension is new to ipywidgets 5.0. It is a dependency of ipywidgets, maybe that's what you mean by included.

@pelson
Copy link
Member

pelson commented Apr 27, 2016

We need a new conda-forge feedstock called widgetsnbextension, and to add that as a dependency here AFAICT. It would also be nice if we had a test in this feedstock which failed without that dependency.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 27, 2016

We need a new conda-forge feedstock called widgetsnbextension

I have a PR almost ready for that. I was just waiting for the CircleCI issues to be resolved. I am sending it soon.

@jakirkham
Copy link
Member

I have a PR almost ready for that. I was just waiting for the CircleCI issues to be resolved. I am sending it soon.

Feel free to send it when you are ready. I don't know what timescale these CircleCI API issues will be resolved. If we need it really badly, we can just restart Travis loads of times until it works. Not optimal, but that is sort of the situation until CircleCI gets back to us.

@jakirkham
Copy link
Member

jakirkham commented Apr 27, 2016

NVM guess you just did.

@jakirkham
Copy link
Member

jakirkham commented Apr 28, 2016

This needs to be re-rendered with conda-smithy built with this commit ( conda-forge/conda-smithy@41e300e ).

@jakirkham
Copy link
Member

There is now a feedstock for widgetsnbextension and packages have been released for all OSes, Python versions, and architectures.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 28, 2016

@pelson not sure if this build is 100% test to ensure we can close #1

I appreciate if someone more knowledgeable of ipywidgets could take a look.


test:
imports:
- ipywidgets

about:
home: http://github.com/jakevdp/ipywidgets
Copy link
Member

Choose a reason for hiding this comment

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

😆 - how did that happen! 😄

Copy link
Member

Choose a reason for hiding this comment

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

Well, he did have something like that once. ( http://github.com/jakevdp/ipywidgets-static )

@pelson
Copy link
Member

pelson commented Apr 28, 2016

This is good to go. Thanks @ocefpaf!
@jdfreder - do you have a way of testing that the widgetsnbextension is available (I guess we could always just try importing it).

@parente
Copy link
Member

parente commented May 1, 2016

ipywidgets is now at version 5.1.2 to fix some bugs an API incompatibilities. And widgetsnbextension is at 1.2.1. Does it make sense to go to 5.0.0 first and then bump again or just update this PR?

@jakirkham
Copy link
Member

Nice to see you over here, @parente. At this point, I'm thinking conda-forge is the only way to keep up with Jupyter's rapid releases in any realistic way. I wonder if it would be worthwhile to start adding conda-forge as a channel in the docker images so that we can ensure we are getting the newest packages available.

Sure, that's fine. Right now I think we are waiting for some feedback from, @jdfreder. There was some concern that ipywidgets was not getting installed and configured correctly before and we would like to know if we are on the mark now or if there is more we need to fix before merging this. Related, we would like to have some test to ensure we are now installing it correct and will continue to be in future releases.

@parente
Copy link
Member

parente commented May 1, 2016

I wonder if it would be worthwhile to start adding conda-forge as a channel in the docker images so that we can ensure we are getting the newest packages available.

@minrk added it yesterday to docker-stacks and @willingc merged. I don't think a build was kicked off though.

Sure, that's fine. Right now I think we are waiting for some feedback from, @jdfreder.

Understood. Just a friendly FYI on the versions.

@ocefpaf
Copy link
Member Author

ocefpaf commented May 1, 2016

Does it make sense to go to 5.0.0 first and then bump again or just update this PR?

I would just update the PR. Oh wait! I am the author of the PR 😳

Let me get to it...

@jakirkham
Copy link
Member

@minrk added it yesterday to docker-stacks and @willingc merged. I don't think a build was kicked off though.

Awesome!

Sorry, I may have missed it. My notifications are literally swamped with stuff from here. Still trying to figure out how best to handle that better.

Understood. Just a friendly FYI on the versions.

Sure and we appreciate the FYI.

@SylvainCorlay
Copy link
Member

We will probably make minor bug-fix releases of ipywidgets and
widgetsnbextension quite often from now on.

IMO there are only benefits to updating the PR.
On Apr 30, 2016 8:18 PM, "jakirkham" [email protected] wrote:

@minrk https://github.com/minrk added it yesterday to docker-stacks and
@willingc https://github.com/willingc merged. I don't think a build was
kicked off though.

Awesome!

Sorry, I may have missed it. My notifications are literally swamped with
stuff from here. Still trying to figure out how best to handle that better.

Understood. Just a friendly FYI on the versions.

Sure and we appreciate the FYI.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3 (comment)

@ocefpaf
Copy link
Member Author

ocefpaf commented May 1, 2016

@ocefpaf Sure 😄

Done! Welcome to conda-forge!!

@@ -59,10 +59,15 @@ install:
- cmd: set PATH=%CONDA_INSTALL_LOCN%;%CONDA_INSTALL_LOCN%\scripts;%PATH%
- cmd: set PYTHONUNBUFFERED=1

- cmd: conda config --set show_channel_urls true
- cmd: conda install -c http://conda.binstar.org/pelson/channel/development --yes --quiet obvious-ci
- cmd: conda config --add channels http://conda.binstar.org/conda-forge
- cmd: conda info
- cmd: conda install -n root --quiet --yes conda-build anaconda-client jinja2 setuptools
Copy link
Contributor

Choose a reason for hiding this comment

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

Also pin the version of conda-build, as in .travis.yml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see the line below!

@bollwyvl
Copy link
Contributor

bollwyvl commented May 2, 2016

Also, would be curious to get @bollwyvl's feedback on this change to ipywidgets.

@jakirkham what are you looking for in particular?

I reviewed and didn't see anything out of place, other than my failure to read re: pinning conda-build!

Raised a question over here about the installation procedure for the user:
conda-forge/widgetsnbextension-feedstock#4

@parente
Copy link
Member

parente commented May 2, 2016

@ocefpaf said:

@SylvainCorlay, @parente, and @willingc would you be willing to be added as maintainers of this recipe? If so I can do that in this PR for you.

I'm not an ipywidgets expert by any means, so I think you're better off keeping me on the list of people who send PRs instead of people with the ability to merge and break master. 🙈

@jakirkham
Copy link
Member

If you ( @parente ) are interested in helping maintain the docker image though, we would be more than happy to have you on board there. Though it is substantially simpler than the ones at docker-stacks. Mainly this determines how builds are done on the Linux side of things here.

@SylvainCorlay
Copy link
Member

I'm not an ipywidgets expert by any means, so I think you're better off keeping me on the list of people who send PRs instead of people with the ability to merge and break master. 🙈

@parente is totally an ipywidgets expert.

@jakirkham
Copy link
Member

Ah so he's being humble here. 😉

@jakirkham
Copy link
Member

@jakirkham what are you looking for in particular?

Well @bollwyvl, I remember you had mentioned spending a fair bit of time trying to get these to install properly and I wanted to make sure that the installation we are doing here and with widgetsnbextension looked reasonable. Also, if there are any thoughts you have on good tests, we would appreciate hearing them so that we can verify this continues to work in the future.

@@ -1,17 +1,17 @@
{% set version = "4.1.1" %}
{% set version = "5.1.2" %}
Copy link
Member

Choose a reason for hiding this comment

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

Can you point to 5.1.3, I just made a patch release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Thanks @SylvainCorlay!

@SylvainCorlay
Copy link
Member

This looks good to me. Should we get this in?

@bollwyvl
Copy link
Contributor

bollwyvl commented May 4, 2016

Also, if there are any thoughts you have on good tests, we would appreciate hearing them so that we can verify this continues to work in the future.

Yeah, that's the rub!

On many of our extensions, I am using the somewhat-creaky PhantomJS/CasperJS test stuff that the 4.x notebook uses. It looks like widgets is now on the karma side of things, which I haven't been able to use, as anaconda.org doesn't have native browsers (coming soon, I'm promised)... and if using phantom (even 2.x), would inherit all the same old nasty problems. But it does seem the way of justice.

Assuming we don't have browsers, I guess the best one could do would be to see if the extensions actually installed correctly (i.e. check the output of jupyter (server|nb)extension for errors).

Sorry, going a bit off topic here...

At the Jupyter Dev meeting, we talked about extracting the good bits of jupyter-js-notebook's tests into something that a developer could use at test time to easily employ best practices for testing extension-related functionality in as close to a representative environment as possible. Since this will inevitably use node stuff, conda is a great target, as you can now just pull nodejs off the defaults channel... or use the conda-forge one! We've talked a bit about making common web tool chains installable directly as conda packages, which would be wonderful for cutting down on build times.

It should be possible for a dev to specify:

  • really robust config for the server startup, good logging, etc.
  • notebooks that should have expected output, a la nosebook
  • simple js features
  • easy mocks of "fancy things"
  • screenshots of test steps
    • 🍰 posting (anonymous gist?) of a notebook test report with said screenshots embedded!


test:
imports:
- ipywidgets

about:
home: http://github.com/jakevdp/ipywidgets
home: https://github.com/ipython/ipywidgets
license: BSD 3-clause
summary: IPython Static Widgets
Copy link
Member

@SylvainCorlay SylvainCorlay May 4, 2016

Choose a reason for hiding this comment

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

Should this rather be Jupyter interactive widgets

Copy link
Member Author

Choose a reason for hiding this comment

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

The GitHub pages has: IPython widgets for the Jupyter Notebook I am changing here to Widgets for the Jupyter Notebook, sounds good?

Copy link
Member

Choose a reason for hiding this comment

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

As long as they aren't "static" and it's referring to "Jupyter", I'm happy. @SylvainCorlay comments?

Copy link
Member

Choose a reason for hiding this comment

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

The front-end is now kernel-language agnostic and we are working hard to make it work outside of the notebook....

@SylvainCorlay
Copy link
Member

I am 👍 on merging this and iterating from there...

@SylvainCorlay
Copy link
Member

(I will depend on this for the recipes on ipyleaflet, bqplot, pythreejs...)

@jakirkham
Copy link
Member

I see @bollwyvl. Thanks for the useful feedback. Can we maybe add an issue wrt to testing and we will table it for now?

If people are happy with this as is, I'm eager to see it go in ASAP.

@jakirkham
Copy link
Member

There was a failure downloading miniconda in one of the Travis CI builds. As this is unrelated to the content here, I went ahead and restarted it.

@SylvainCorlay
Copy link
Member

Merge?

@jakirkham
Copy link
Member

Agreed.

@jakirkham jakirkham merged commit 7113b31 into conda-forge:master May 5, 2016
@jakirkham
Copy link
Member

Thanks everyone. If we have new issues, let's iterate. Sound good?

@ocefpaf ocefpaf deleted the bump branch May 5, 2016 14:44
@pelson
Copy link
Member

pelson commented May 6, 2016

I guess the best one could do would be to see if the extensions actually installed correctly (i.e. check the output of jupyter (server|nb)extension for errors).

This is an excellent suggestion, I'd love to see this in the recipe!

Great to have you guys on board with this recipe - welcome! There are details here that only somebody in the thick of development can know so it is awesome to be able to pull on that knowledge to package ipywidgets (& friends) to give conda users the best possible experience.

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.

10 participants