Skip to content

gh-121607: Edited source file import recipe to make it more clear #121519

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Sep 13, 2024
Merged

gh-121607: Edited source file import recipe to make it more clear #121519

Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
08917bd
edited source file import recipe to make it more clear
ChrisBarker-NOAA Jul 8, 2024
bab2fa8
added a note about deprecation of the load_module() method.
ChrisBarker-NOAA Jul 8, 2024
81885ad
Update Doc/library/importlib.rst
ChrisBarker-NOAA Jul 10, 2024
7905ab0
Update Doc/library/importlib.rst
ChrisBarker-NOAA Jul 10, 2024
c78a1a6
removed the note directive.
ChrisBarker-NOAA Jul 10, 2024
b9cb5d3
Update importlib.rst
ChrisBarker-NOAA Jul 10, 2024
73e4c94
added intro paragraph to Importing a source file directly recipe with…
ChrisBarker-NOAA Jul 15, 2024
fed1797
Update Doc/library/importlib.rst
ChrisBarker-NOAA Jul 15, 2024
e5a864e
Update Doc/library/importlib.rst
ChrisBarker-NOAA Jul 15, 2024
e6a1965
Wrap lines to 80 columns
brettcannon Sep 13, 2024
5a74186
Merge branch 'main' into importlib_docs
brettcannon Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions Doc/library/importlib.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1584,20 +1584,29 @@ Note that if ``name`` is a submodule (contains a dot),
Importing a source file directly
''''''''''''''''''''''''''''''''

``SourceFileLoader.load_module()`` has been deprecated -- this recipe should be used instead.
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed to be a warning about the recipe and to consider alternatives before using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't feel qualified to write that warning -- I suppose I could take some of @ncoghlan's note above and try, but maybe someone else could propose text.

FWIW, I guess ncoghlan's note is why this is a recipe in the docs, and not a function in importlib, but I DO think this is a useful thing to do -- yes, i suppose a bit of a foot gun, but still useful.

In regards to the potential double import problem, in this case of the json module - yes, using the recipe for something on the import path is a bad idea, for sure -- that's only there to provide a example that can be run -- I borrowed that from the existing docs, but maybe it would be better to not provide a runnable example, and rather something like:

my_mod = import_from_path("my_mod", "path/to/a_python_file.py")

For the use-case that got me here, I'm using Python as a configuration definition -- yes, very dangerous if you are getting the config from an untrusted source, but that's not the case here (internal data analysis tool) -- and I very much do want a module, not a dict of a namespace.

I think this would be very useful for, e.g. a scriptable application as well, so users could provide external python scripts that could be run.

But, as @ZeroIntensity said, this is already in the docs -- I'm just trying to make it a bit more clear.

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be very useful for, e.g. a scriptable application as well, so users could provide external python scripts that could be run.

In most cases, runpy can do that, but that's beside the point anyway.

Copy link
Member

Choose a reason for hiding this comment

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

You can take what I outlined as what should be in the paragraph and just write better prose:

I think it would be better to have a sentence or paragraph pointing out that the recipe is an approximation of an import statement where you specify the file path. It should also point out that modifying sys.path may be better than circumventing the import system or using runpy.run_path() if you just need the global namespace and not a module object.


To import a Python source file directly, use the following recipe::

import importlib.util
import sys
import importlib.util
import sys

# For illustrative purposes.
import tokenize
file_path = tokenize.__file__
module_name = tokenize.__name__

spec = importlib.util.spec_from_file_location(module_name, file_path)
module = importlib.util.module_from_spec(spec)
sys.modules[module_name] = module
spec.loader.exec_module(module)

def import_from_path(module_name, file_path):
spec = importlib.util.spec_from_file_location(module_name, file_path)
module = importlib.util.module_from_spec(spec)
sys.modules[module_name] = module
spec.loader.exec_module(module)
return module


# For illustrative purposes only (use of `json` is arbitrary).
import json
file_path = json.__file__
module_name = json.__name__

# Similar outcome as `import json`.
json = import_from_path(module_name, file_path)
Comment on lines +1609 to +1614
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included this example because there had been a similar one in the previous version.

However, it is (always?) a bad idea to use this recipe to import a module on sys.path, particularly an stdlib one -- so maybe better to simply not provide a runnable example?

I expect the code itself is clear enough on how it can be used.



Implementing lazy imports
Expand Down