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

Fix detection of dependencies where key and name differ #36

Merged
merged 14 commits into from
Jul 24, 2024
Merged

Conversation

wch
Copy link
Collaborator

@wch wch commented Jul 24, 2024

This should fix posit-dev/py-shiny-site#186.

For some packages, the entry in pyodide-lock.json has a key like jsonschema-specifications, but in that entry, the value name field is jsonschema_specifications. (And further note that these can differ from the module name, which is listed in imports). As far as I can tell, there are no fixed rules for how these names relate.

Here are some entries in pyodide-lock.json where these things are not consistent (I've removed extraneous information from these entries to make it easy to see the inconsistencies):

    "jsonschema-specifications": {
      "depends": [],
      "imports": ["jsonschema_specifications"],
      "name": "jsonschema_specifications",
    }

    "jsonschema-specifications-tests": {
      "depends": ["jsonschema_specifications"],
      "imports": [],
      "name": "jsonschema_specifications-tests",
    }


    "jinja2": {
      "depends": ["markupsafe"],
      "imports": ["jinja2"],
      "name": "Jinja2",
    }

    // In this case the `imports` entry differs from the `name` entry and the key.
    "opencv-python": {
      "depends": ["numpy"],
      "imports": ["cv2"],
      "name": "opencv-python",
    }

We already handled the differing module name before. This PR adds support for differences between the key and the name.

@wch wch requested a review from cpsievert July 24, 2024 18:19
@wch
Copy link
Collaborator Author

wch commented Jul 24, 2024

I just learned that there appears to be one exception to how this all works, and that is jinja2:

    "jinja2": {
      "depends": ["markupsafe"],
      "imports": ["jinja2"],
      "name": "Jinja2",
    }

This is listed as a dependency with the name jinja2. For example:

    "pandas": {
      "depends": ["numpy", "python-dateutil", "pytz", "jinja2"],
      "imports": ["pandas"],
      "name": "pandas",
    }

After seeing this, I thought that maybe the module name (listed in imports) was the thing to use for imports. But no, that can't be right either, because the things listed in depends can have a hyphen (as in python-dateutil above), and modules in Python can't have hypens in their name. So I think we may need to special case jinja2/Jinja2.

@wch
Copy link
Collaborator Author

wch commented Jul 24, 2024

It looks like package names should be handled in a case-insensitive manner, so I implemented that and it solves the jinja2/Jinja2 issue.

pyodide/pyodide#1614

@wch
Copy link
Collaborator Author

wch commented Jul 24, 2024

Regarding posit-dev/py-shiny-site#186, this reduces the necessary code to the following:

app.py

from shiny.express import input, ui
from shinywidgets import render_altair

ui.input_selectize("var", "Select variable", choices=["bill_length_mm", "body_mass_g"])


@render_altair
def hist():
    import altair as alt
    from palmerpenguins import load_penguins

    df = load_penguins()
    return (
        alt.Chart(df)
        .mark_bar()
        .encode(x=alt.X(f"{input.var()}:Q", bin=True), y="count()")
    )

requirements.txt

anywidget

No more soft_dependencies.py, and most packages are removed from requirements.txt.

If you run the following, and then visit the app, it works:

shinylive export appdir out  &&  python3 -m http.server --directory out --bind localhost 8008

@wch
Copy link
Collaborator Author

wch commented Jul 24, 2024

Note that we should have tests of dependency detection.

shinylive/_deps.py Outdated Show resolved Hide resolved
Comment on lines +457 to +458
if name not in _dep_name_to_dep_key_mappings():
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this maybe throw a warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nvm, now I see the warning above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be fairly common to not have a package in the pyodide_lock.json file. In that case, micropip will go out and try to install it from PyPI.

@wch wch merged commit 6df837f into main Jul 24, 2024
6 checks passed
@wch wch deleted the fix-dep-names-keys branch July 24, 2024 21:17
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.

2 participants