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

Start best practices notebook #6819

Merged
merged 14 commits into from
Sep 16, 2024
Merged

Start best practices notebook #6819

merged 14 commits into from
Sep 16, 2024

Conversation

ahuang11
Copy link
Contributor

@ahuang11 ahuang11 commented May 9, 2024

Starts migrating the collection from https://discourse.holoviz.org/t/personal-opinions-about-best-practices-for-panel-holoviews/6789/27 to the official docs page, with examples

Thus far, I think it can be broken down into two sections: Dev Experience (or I was also thinking 'Nice to knows') and User Experience, and thus two separate notebooks.

Feedback appreciated on how to proceed (and what's missing; in particular, how to add it to the index).

@Coderambling
Copy link
Contributor

Coderambling commented May 10, 2024

Great iniative @ahuang11 ! As there are so many ways to do things in Panel, this is very useful (and necessary).

Regarding the recommendation to use pn.viewer.viewable instead of param; maybe add a (link to) an explanation as to why this is better?

See same question here:

https://discourse.holoviz.org/t/difference-between-param-parameterized-and-pn-viewable-viewer/6105

@ahuang11
Copy link
Contributor Author

ahuang11 commented May 10, 2024

Thanks for the feedback; links sprinkled within are good ideas.

I believe Viewer just makes it behave like a Panel object, i.e. include show(), servable(), etc. https://panel.holoviz.org/how_to/custom_components/custom_viewer.html

Check out the source:
https://github.com/holoviz/panel/blob/main/panel/viewable.py#L1024-L1071

@Coderambling
Copy link
Contributor

Coderambling commented May 10, 2024

Ok! Maybe include that link to the source in the Notebook as well?

Are you planning to make collapsible sections and subsections in the Notebook?

Will it be one Notebook with dev and user main sections, or 2 Notebooks? I would suggest 1 for now, otherwise in intro of each Notebook, mention a link to the other?

@Coderambling
Copy link
Contributor

Include a section on when (not) to use .rx, and instead of what?

@ahuang11
Copy link
Contributor Author

Are you planning to make collapsible sections and subsections in the Notebook?

No, I think this will be in the docs.

Will it be one Notebook with dev and user main sections, or 2 Notebooks? I would suggest 1 for now, otherwise in intro of each Notebook, mention a link to the other?

I believe this will be separate notebooks eventually.

@MarcSkovMadsen
Copy link
Collaborator

I'm positive on this PR.

I Think it would help shaping this with a motivating feature request. Why do we need this? What are we trying to achieve?

I also Think it would help to explain where in the docs this fits. Is it an Explanation? Is it a tutorial? Or ?

Personally i would prefer to write in a markdown file instead of notebook.

@Coderambling
Copy link
Contributor

Coderambling commented May 15, 2024

But Notebooks can contain interactive examples? Of both things that go right and go wrong. I find that quite useful.

On where it fits: good question. I like the Best Practices title. Or best practices: does and don'ts.

I think it should be somewhere in the Getting Started neighbourhood, because:

-It can help in preventing new users to go down a wrong path.

-It helps to get a "feel" for Panel.

If it achieves those 2 things, then that is a good justification.

Also, it provides a place to accumulate (verified and vetted) similar code snippets and advice in one place, after interactive discussion and iteration has taken place in more fluid places like Discord and Discourse.

@Coderambling
Copy link
Contributor

Coderambling commented May 15, 2024

But Notebooks can contain interactive examples? Of both things that go right and go wrong. I find that quite useful.

On where it fits: good question. I like the Best Practices title. Or best practices: do'ss and don'ts.

I think it should be somewhere in the Getting Started neighbourhood, because:

-It can help in preventing new users to go down a wrong path.

-It helps to get a "feel" for Panel.

If it can achieve those things, that's already worth it.

Also, it provides a place to accumulate similar code snippets and advice from more fluid places like Discord and Discourse, after discussions and iterations have taken place there.

Like the below for example:-)

@Coderambling
Copy link
Contributor

Coderambling commented May 15, 2024

Hi @ahuang11 . How about including the excellent answer below by Jerry Vinokurov to the Best Practices? Do you think it is a good fit?

https://discourse.holoviz.org/t/what-is-meant-by-avoid-modifying-the-objects-parameter-directly/7234

@ahuang11
Copy link
Contributor Author

How about including the excellent answer below by Jerry Vinokurov to the Best Practices?

Added it!

@ahuang11 ahuang11 marked this pull request as ready for review July 12, 2024 22:04
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.18%. Comparing base (19025e1) to head (3f0dd80).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6819      +/-   ##
==========================================
- Coverage   82.19%   82.18%   -0.01%     
==========================================
  Files         337      337              
  Lines       50363    50380      +17     
==========================================
+ Hits        41398    41407       +9     
- Misses       8965     8973       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Coderambling
Copy link
Contributor

Coderambling commented Jul 30, 2024

Looks good already! How about adding the below pointed out by Droumis?

It points out an issue that many people could run into, and @jbednar has already added a short (5 lines) code example in the Discord thread that illustrates it.

He has also added 2 images.

https://discord.com/channels/1075331058024861767/1265376235979018423

Code example by James:

I don't know if this is minimal, but it is sufficient:

import pandas as pd, hvplot.pandas

df = pd.DataFrame(dict(x=[1,2,3], y=[10, 2, 15]))
p = df.hvplot(rasterize=True)
p

See his explanation below. I have attached the 2 images of the plots that he provided on Discord.

image-1.png

p

image-2.png

Here zooming and panning in the second plot is working fine, but the first plot tries to update to what the second plot is doing, and only partially updates.

If it all updated to match the second plot it would be ok, as Panel widgets do (all widgets update when one is dragged, regardless of which cell the widget is in).

Link to the text: https://discord.com/channels/1075331058024861767/1265376235979018423/1266966940992278580

Original description of problem by Droumis:

In trying to build a notebook that incrementally displays steps towards building an app. Displaying multiple instances of the same plots/widgets (even if cloned) seems to be fragile and lead to interference issues when working with callbacks and other interactivity. So I though maybe I could just generate a png of the HoloViews + Panel components on the fly. HoloViews makes this possible (using selenium, firefox, geckodriver) with hv.output(element, fig='png') ... Does panel have something similar so I could include a snapshot of the holoviews elements with the panel components without saving a png to file?

Link:

https://discord.com/channels/1075331058024861767/1101491338899370014/1265376235979018423

@Coderambling
Copy link
Contributor

Coderambling commented Jul 30, 2024

Regarding the phrase below:

This leaves the question: in what cases DO you inherit from param.Parameterized?

So maybe clarify by adding at the end: (Only) inherit from param.Parameterized when you... / in cases where...

Or maybe also at the start add some context:

When you are developing / building x, y, z, instead of inheriting from param.Parameterized... etc.

'Instead of inheriting from param.Parameterized, opting for inheritance from pn.viewable.Viewer allows direct invocation of the class, resembling a native Panel object'

@Coderambling
Copy link
Contributor

The below is being drafted. Could be relevant to put a link to that in the doc?

#7043

@ahuang11 ahuang11 added this to the v1.5.0 milestone Aug 6, 2024
@ahuang11 ahuang11 marked this pull request as draft August 27, 2024 18:06
@ahuang11 ahuang11 marked this pull request as ready for review August 28, 2024 01:48
@Coderambling
Copy link
Contributor

Coderambling commented Aug 28, 2024

I got the following reply from a user a couple of days ago, after I pointed him to the draft best practices document, and asked him for feedback. (I asked him if it was ok to share it and he indicated it was)

Based on this feedback the best practices document clearly fills a need, particularly for new users.

Would make sense to have a link to this doc in Getting Started, to help new users avoid taking wrong turns.

This case seems an interesting illustration of what a user new to Panel can encounter in terms of learning curve.

Would be great if more feedback like this could be collected from new users.

For more context, See the Discord threads for the problem he had and the solution that was found.

Problem:

https://discord.com/channels/1075331058024861767/1088157184489164831/1261650883373174784

Solution: (he changed from functions to using parametrized classes to fix it).

https://discord.com/channels/1075331058024861767/1261651824377724958

Hello there <@1205489736257372211> , I saw the guide and it was quite useful although most of the things I had learnt to do the hard way lol. But this is a great addition to the panel documentation, really like it. This would have definitely saved much time and frustration if I would have read this earlier. Is it already merged with the published documentation? Because I couldn't find it when going through the docs!

Request to provide feedback:

This was in response to my question:

Hi <@695299667201622088> . Interesting to read your approach to solving your issue in the thread below.

https://discord.com/channels/1075331058024861767/1261651824377724958

If you have time at some point, could you have a quick look at the first draft of the Best Practices / Do's and Don'ts guide on the Panel Github?

Hopefully some of the stuff in there will be immediately useful for you!

Could you give me some general feedback if you think it's useful, and if, generally, you recognize / have come across some the situations it describes?

Would it have helped to solve your issue?

You should be able to view / read it online right from the below url.

https://github.com/holoviz/panel/blob/add_best_practices/examples%2Fbest_practices.ipynb

@ahuang11
Copy link
Contributor Author

Not sure if it's best placed in getting started, or how to.

@philippjfr
Copy link
Member

Use batch_call_watchers to update multiple params on separate objects simultaneously.

Afaik this does nothing, batching only works at the level of a parameterized never across multiple objects.

@philippjfr
Copy link
Member

Refreshing layout objects

This should mention using the APIs on the Column itself, e.g.:

# instead of
col.objects[0] = 'Foo'
# do
col[0] = 'Foo'

@Coderambling
Copy link
Contributor

Not sure if it's best placed in getting started, or how to.

I would say in getting started, as it is new users that benefit the most from avoiding wrong turns.

Assuming here that people will check out Getting Started before looking at How To.

@Coderambling
Copy link
Contributor

Coderambling commented Sep 4, 2024

Should the below be added to Best Practices?

I.e:

Do: Use CompositeWidget to create custom Panel components.

Don't: Use the Viewer for this.

I've been pointing out multiple times that its very hard to create real Panel widgets from the Viewer. Andrew then told me in #7030 that I should be using the CompositeWidget.

Reference:

#7051

@ahuang11 ahuang11 mentioned this pull request Sep 9, 2024
28 tasks
@philippjfr philippjfr merged commit 919d996 into main Sep 16, 2024
1 check was pending
@philippjfr philippjfr deleted the add_best_practices branch September 16, 2024 08:56
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.

4 participants