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

Enable Binder #2198

Merged
merged 71 commits into from
May 12, 2021
Merged

Enable Binder #2198

merged 71 commits into from
May 12, 2021

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Apr 17, 2021

Address #1500, #1369 (comment) and #2218.

This will enable opening the Panel examples on Binder. Whether it will also be possible to add a binder link to all examples is unknown and "nice to have" for this PR.

image

For now you can access binder via https://mybinder.org/v2/gh/holoviz/panel/binder?urlpath=lab/tree/examples

I would like a suggestion from @philippjfr or @jbednar which link we should provide when released. I see the following options

  • Link to binder branch.
    • Pros: We can control what is available on Binder
    • Cons: We will have to remember and spend time on updating the binder branch
  • Link to master branch.
    • Pros: Everything is always the latest
    • Cons: There might be things is master that changes the behaviour of the current version of Panel released.
  • Link to release branch of Panel. I.e. for example the version for the Panel 0.11.3 release
    • Pros: The current version of Panel is available on Binder
    • Cons:
      • Will have to remember to update links across the repository.
      • Users having bookmarked the link would stay on that version of Panel on Binder

@MarcSkovMadsen
Copy link
Collaborator Author

Status: Things are starting to work. I'm finding bugs in the Reference Examples. I have to update the gallery notebooks in order to be able to serve them as apps. I would like to configure VS Code to enable showcasing how powerful the combination of Panel and VS Code is.... Also on Binder and in a Jupyter Hub.

panel-binder.mp4

So I would really, really like Panel to work in VS Code. But it requires fixing bokeh/jupyter_bokeh#131

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Apr 20, 2021

Hi @philippjfr

As I see it 1) Binder is working 2) It now comes with a nice setup like panel-jupyter-proxy, panel serve of all gallery examples, vs code integrated and a new, improved index page.

In principle this could be all for this PR except adding a link to binder in different places. For example the README.

BUT all the notebooks need to be walked through and prettified in order to be useable as apps. This could be a lot of work and a lot of minor changes. And to be honest. Even without binder a test and update of the notebooks could be in place (c.f. all the bugs posted, I have just scratched the surface I believe).

Here are my questions.

  • Do you want the gallery notebooks updated in general to be responsive and end up in a nice app template?
  • Do you want the reference examples updated in general to be responsive and end up in a nice app template?`
  • If yes to some of these questions. Do you want separate PRs for each notebook, one large Notebook PR or just this PR with everything?
  • Could you answer the question about which link to provide to users posted in the first post? Thanks.

@philippjfr
Copy link
Member

Do you want the gallery notebooks updated in general to be responsive and end up in a nice app template?

No, some of them sure, but definitely not all. The focus should be the feature being demonstrated not making all the examples look like.

Do you want the reference examples updated in general to be responsive and end up in a nice app template?`

Definitely not, in fact I'd argue there's no reason to make these available as apps at all.

Could you answer the question about which link to provide to users posted in the first post? Thanks.

I'll make sure to configure this once you're done here. The link will go in the interactivity warning that floats on the right side.

@philippjfr
Copy link
Member

If yes to some of these questions. Do you want separate PRs for each notebook, one large Notebook PR or just this PR with everything?

Just make a single PR for any coordinated changes like this.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented May 2, 2021

Binder is now working as I originally intended it to and more :-).

Some things are not in scope for me

  • Adding links from every example at panel.holoviz.org to the notebook and app on Binder. I hope @philippjfr can/ will do this.

Some things are not working and I am not capable of getting it working

Working Video

binder-gallery-works.mp4

Not Working

Watch this video on Youtube https://www.youtube.com/watch?v=gI3qQRVdIIE

@philippjfr Feel free to review and start creating value from this :-) I don't plan to do more on this before getting a review and feedback.

Thanks.

@MarcSkovMadsen MarcSkovMadsen changed the title Enable Binder - WORK IN PROGRESS Enable Binder May 2, 2021
@MarcSkovMadsen MarcSkovMadsen requested a review from philippjfr May 2, 2021 09:31
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Nice! This will make things look and feel much better. Should we promote the approach from #2271 or is it better to put the template separately like it is here?

Xvfb :99 -screen 0 1280x1024x24 > /dev/null 2>&1 &
sleep 3
jupyter trust examples/**/*.ipynb
jupyter trust examples/**/**/*.ipynb
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a bash expert, but doesn't ** indicate recursive directory searching nowadays? If so, isn't the second trust call redundant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought so. But it did not work like that for me. So I added it twice. That works.

examples/gallery/apis/stocks_altair.ipynb Outdated Show resolved Hide resolved
@MarcSkovMadsen
Copy link
Collaborator Author

Nice! This will make things look and feel much better. Should we promote the approach from #2271 or is it better to put the template separately like it is here?

When the new approach #2271 is ready and tested I think it would make a lot of sense to switch. That syntax is nice and shorter.

The only downside is that then not a lot of examples will show how to build apps with templates for non-notebook users. There the natural workflow might still be to explicitly declare a template ??

@jbednar
Copy link
Member

jbednar commented May 3, 2021

Good point. We should still have a separate example of building such a template explicitly somewhere, and while it's true people may not see that example, having people use servable unnecessarily in a standalone file is not that terrible. I.e., I agree it doesn't make as much sense outside a notebook, but it's still going to be fine for many people, and so I'm ok with more advanced non-notebook users having to do a bit more work to find that there is this other way.

setup.py Outdated
Comment on lines 153 to 157
'pydeck',
'pyecharts',
'idom',
'xlsxwriter',
'pyarrow',
Copy link
Member

Choose a reason for hiding this comment

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

Most of these cannot be added to setup.py since that is also the ground truth for conda installs. If I remove these here will this break the binder setup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. But we can add them to one of the files in the binder subfolder. Can you point out which should not be included?

Copy link
Member

Choose a reason for hiding this comment

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

Already removed all the ones you added. Anything that is not conda installable cannot appear here, so for now I've removed pydeck, pyecharts, idom, xlsxwriter and pyarrow. You could readd pyarrow since that is available.

setup.py Outdated Show resolved Hide resolved
philippjfr and others added 3 commits May 3, 2021 12:05
Co-authored-by: James A. Bednar <jbednar@users.noreply.github.com>
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #2198 (bed6e07) into master (9984f55) will increase coverage by 0.38%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2198      +/-   ##
==========================================
+ Coverage   83.64%   84.02%   +0.38%     
==========================================
  Files         183      181       -2     
  Lines       21947    21940       -7     
==========================================
+ Hits        18357    18436      +79     
+ Misses       3590     3504      -86     
Impacted Files Coverage Δ
panel/tests/pane/test_alert.py 90.90% <0.00%> (ø)
...nel/tests/template/fast/test_fast_grid_template.py 95.83% <0.00%> (-2.09%) ⬇️
...nel/tests/template/fast/test_fast_list_template.py 87.50% <0.00%> (-6.25%) ⬇️
panel/tests/template/test_manual.py 61.61% <0.00%> (-1.39%) ⬇️
panel/tests/template/test_vanilla_manual.py 86.95% <0.00%> (-2.41%) ⬇️
panel/tests/widgets/test_speech_to_text.py 90.72% <0.00%> (ø)
panel/tests/widgets/test_text_to_speech.py 92.50% <0.00%> (ø)
panel/tests/pane/test_markup.py 99.33% <100.00%> (ø)
panel/tests/widgets/test_tables.py 99.23% <100.00%> (ø)
panel/config.py 51.78% <0.00%> (-0.16%) ⬇️
... and 8 more

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 9984f55...bed6e07. Read the comment docs.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented May 3, 2021

There is one fundamental thing to consider. Which branch should be promoted and shared with users? In principle any branch will be available on Binder. But it's important to agree on one that is promoted.

My recommendation would be to promote the binder branch. Then it can be updated and fixed whenever there is a new release. The master branch for example will not always be working is my experience. For example the .ts code cannot build right now I believe.

@philippjfr
Copy link
Member

For example the .ts code cannot build right now I believe.

There are a few errors but they are not preventing the .ts from being built and don't cause any real issues. However I should add a CI check to ensure there are zero TS errors or warnings. Not entirely excited about a binder branch since that's an additional release step but given all the alternatives that still seems like the best approach.

@MarcSkovMadsen
Copy link
Collaborator Author

The alternative is just using the master branch. But then users would see functionality that is not yet released. That might be confusing. But maybe also just fine.

@jbednar
Copy link
Member

jbednar commented May 3, 2021

Sound to me like it ought to be using a tag indicating the latest release, rather than a separate branch? Our release workflow would then need to update latest-release or released whenever we make a full (non-dev) release. Seeing changes post-release would not only be confusing, it would likely not work, if it's anything like when I try Panel outside the release process (due to the JS build/publish process involved).

@philippjfr philippjfr merged commit 985a180 into master May 12, 2021
@philippjfr philippjfr deleted the binder branch May 12, 2021 09:50
@philippjfr
Copy link
Member

Thanks again for this extraordinary effort @MarcSkovMadsen. I'll be adding binder links to all pages in a separate PR.

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.

3 participants