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

Switch to using bokeh_sampledata package #13874

Merged
merged 8 commits into from
May 8, 2024
Merged

Conversation

bryevdv
Copy link
Member

@bryevdv bryevdv commented May 6, 2024

fixes: #13856

Initial push to see how CI behaves. cc @philippjfr

@bokeh/dev For simplicity I opted to just mention pip install bokeh_sampledata in most places (error messages especially) but we could expand to include conda if that seems necessary.

Additionally, for now I have just removed everything related to bokeh sampledata altogether. But can add some kind of actionable error message in case users attempt to run that, if desired.

@hoxbro
Copy link
Contributor

hoxbro commented May 6, 2024

This is convenience more than anything else, but LSP, like pylance / pyright (what a lot of VSCode user use), works differently than how IPython does.

LSPs do not actually run the code, so they cannot access the values of __dir__ like IPython does. So if you want to autocomplete from bokeh.sampledata.us_cities import d with an LSP you need to have the actual import (guarded behind TYPE_CHECKING) for autocomplete to work.

Here I have an example without the import:
image

and with the import:
image

Where I have updated us_citites.py to:

from typing import TYPE_CHECKING
from . import _create_sampledata_shim

if TYPE_CHECKING:
    from bokeh_sampledata.us_cities import data  # noqa

__getattr__, __dir__ = _create_sampledata_shim(__name__)

I'm not saying this is needed, but at least something to be aware of.

@bryevdv
Copy link
Member Author

bryevdv commented May 6, 2024

@hoxbro That makes sense, and as an aside, is coincidentally very closely aligned with the new discussion #13870. Would it be possible to take a similar approach as described there, and generate .pyi stubs to include instead? I ask because adding all those guarded imports directly in the main repo modules will make for a very large "manually keep it in sync" surface area, whereas I suspect stub files could be auto-generated.

(I would very much like to have any comments from you in #13870 as well)

@bryevdv
Copy link
Member Author

bryevdv commented May 7, 2024

All the examples seem to be working with the shims, but currently the docs for the shim modules is not:

Screenshot 2024-05-06 at 17 46 38

@bryevdv
Copy link
Member Author

bryevdv commented May 7, 2024

Docs are fixed now, I think this is ready for review. @tcmetzger let me know what you think about any docs needs.

@hoxbro assuming you agree about .pyi file approach, we can add that in another follow-on PR to apply that across the repo wherever it makes sense.

@mattpap mattpap added this to the 3.5 milestone May 7, 2024
@hoxbro
Copy link
Contributor

hoxbro commented May 7, 2024

generate .pyi stubs to include instead?

Yes, this should be possible. I did some initial testing below.

I have created the following folder structure to test it out:

image

Where us_cities.pyi contains the following:

from __future__ import annotations

data: callable

__dir__: list[str]

And that is autocompleting:
image

I think the logic follows PEP-561, and I think adding the stubs folder will take precedence over the bokeh folder.

Without the stubs folder:
image

With the stubs folder:
image

So then I tried to add the us_cities.pyi directly to bokeh/sampledata, which also looks to work.

@bryevdv
Copy link
Member Author

bryevdv commented May 7, 2024

cc @mosc9575 also if you have any comments about the docs or anything else

@bryevdv
Copy link
Member Author

bryevdv commented May 7, 2024

@hoxbro that's really encouraging. I had imagined putting the .pyi files alongside the source files, which seems to be what the PEP suggests, if you are not also creating a stub-only distribution

For package maintainers wishing to ship stub files containing all of their type information, it is preferred that the *.pyi stubs are alongside the corresponding *.py files.

Is there any specific advantages to using the separate bokeh-stubs directory? ("keeping them separate" is both a plus and a minus for organization and maintenance, but wondering if there are any other overriding considerations)

@hoxbro
Copy link
Contributor

hoxbro commented May 7, 2024

I had imagined putting the .pyi files alongside the source files

Sorry, I can see I wasn't clear in my previous post. When using .pyi files alongside the original files. You get the behavior like without the stubs folder in the example above, e.g., you don't need to create a .pyi file for every .py file for the LSP to figure out the structure. So this is what I would do.

Is there any specific advantages to using the separate bokeh-stubs directory?

Not as far as I see. This was just the starting point I took to test things out.

Copy link
Contributor

@mattpap mattpap left a comment

Choose a reason for hiding this comment

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

Looks good. Let's follow up with *.pyi, etc., in future PRs.

@bryevdv
Copy link
Member Author

bryevdv commented May 8, 2024

OK I think there's still some docs tweaks and tests that can be added but let's get this merged so that @philippjfr can experiment with a dev build

@bryevdv bryevdv merged commit db04371 into branch-3.5 May 8, 2024
25 checks passed
@bryevdv bryevdv deleted the bv/remove-sampledata branch May 8, 2024 16:38
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.

Move sampledata files to pip/conda installable package
3 participants