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

Make pyimport able to return module attributes #4395

Merged
merged 11 commits into from Jan 24, 2024

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jan 20, 2024

Before this PR, pyimport can be used like: pyimport("package") or pyimport("package.module") but pyimport("package.attribute") fails. This updates pyimport to also work to get package attributes.

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

Before this PR, `pyimport` can be used like: `pyimport("package")` or
`pyimport("package.module")` but `pyimport("package.attribute")` fails. This
updates `pyimport` to also work to get package attributes.
@hoodmane
Copy link
Member Author

@ryanking13 another possibility is to add an optional second fromlist argument:

pyodide.pyimport(path, fromlist);

So that you could do:

const { PyodideConsole, repr_shorten, BANNER } = pyodide.pyimport(
  "pyodide.console",
  ["PyodideConsole", "repr_shorten", "BANNER"]
);

@hoodmane
Copy link
Member Author

I guess I'll add that here and we can discuss.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks @hoodmane!

For me, pyimport("package.attribute") seems slightly weird as it is not a supported behavior in Python. For instance,

import sys.platform

will raise exception: No module named 'sys.platform'; 'sys' is not a package.

So I am +1 for adding fromlist argument (or probably with some other name).

@hoodmane
Copy link
Member Author

pyimport("package.attribute") seems slightly weird as it is not a supported behavior in Python.

Well we are already deviating from import a.b since if you do __import__("pkg.module") it returns pkg whereas we are returning pkg.module. So we behave more like from pkg import module than import pkg.module already. I think supporting pyimport("pkg.attribute") as well meaning from pkg import attribute actually makes the mental model more consistent. It's also convenient. WDYT?

It is a little bit weird having:

pyimport("pkg.module")            ==> from pkg import module
pyimport("pkg.module", fromlist)  ==> from pkg.module import <fromlist>

but I don't think it's completely crazy.

Also, when we pass a fromlist do you think we should return an Array or an Object?

@ryanking13
Copy link
Member

ryanking13 commented Jan 23, 2024

Well we are already deviating from import a.b since if you do import("pkg.module") it returns pkg whereas we are returning pkg.module.

I see. That makes sense. Then I think pyimport("package.attribute") seems fine to me.

About the fromlist parameter, I just realized that we can already do a similar thing in JavaScript using destructuring assignment syntax. For instance, things like:

const { PyodideConsole, repr_shorten, BANNER } = pyodide.pyimport("pyodide.console")
const { platform, version, path } = pyodide.pyimport("sys")

already works without any modification. So maybe we don't need the fromlist parameter so much?

@hoodmane
Copy link
Member Author

hoodmane commented Jan 23, 2024

we can already do a similar thing in JavaScript using destructuring assignment syntax.

Oh this is a great observation! The only very minor concern is that you create and leak a temporary PyProxy this way, but I think it shouldn't matter:

  1. packages are already globally stored in sys.modules
  2. you won't be creating a reference loop so this will eventually get cleaned out by the JS GC
  3. this is happening in run-once setup code not in loops or anything.

So I will remove the fromlist but keep the attribute change.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks!

@hoodmane hoodmane merged commit 1396c8c into pyodide:main Jan 24, 2024
40 of 41 checks passed
@hoodmane hoodmane deleted the more-flexible-pyimport branch January 24, 2024 23:19
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.

None yet

2 participants