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

Add PyImport_ImportModuleAttr(mod_name, attr_name) helper function #53

Closed
6 tasks done
vstinner opened this issue Jan 19, 2025 · 10 comments
Closed
6 tasks done

Add PyImport_ImportModuleAttr(mod_name, attr_name) helper function #53

vstinner opened this issue Jan 19, 2025 · 10 comments

Comments

@vstinner
Copy link

vstinner commented Jan 19, 2025

Python has a convenient internal _PyImport_GetModuleAttrString(mod_name, attr_name) helper function to import a module and get a module attribute. I propose to make this function public to be able to use it outside Python. I also propose to make its variant _PyImport_GetModuleAttr(mod_name, attr_name) public (which takes objects rather than strings).

API:

PyObject* PyImport_ImportModuleAttr(PyObject *mod_name, PyObject *attr_name)

Import the module mod_name and get its attribute attr_name.

Helper function combining PyImport_Import() and PyObject_GetAttr(). For example, it can raise ImportError if the module is not found, and AttributeError if the attribute doesn't exist.

API:

PyObject* PyImport_ImportModuleAttrString(const char *mod_name, const char *attr_name)

Similar to PyImport_ImportModuleAttr(), but names are UTF-8 encoded strings instead of Python str objects.

For example, PyImport_ImportModuleAttrString("sys", "argv") gets sys.argv. It's similar to from sys import argv in Python.

The function is used by 24 C files in Python.

Links:

Vote:

UPDATE: Rename the functions from PyImport_GetModuleAttr[String]() to PyImport_ImportModuleAttr[String]().

@encukou
Copy link
Collaborator

encukou commented Jan 20, 2025

On the one hand: yes, this sure sounds useful!

On the other hand, the fact that we need a helper to wrap a sequence of a few function calls shows that the C-API has some issues with composability. The error handling is too verbose.
In my mind, there's a layer of the C-API that's only really useful for C itself. These functions would be part of that.

Should we make these inline static and, as the PR already does, document that they're only combining PyImport_Import and PyObject_GetAttr (plus conversions to PyUnicode)?

@zooba
Copy link

zooba commented Jan 20, 2025

Why not PyImport_ImportModuleAttr[String]? Since it's going to do the import, ..._GetModule... isn't as accurate.

I don't think there's anything wrong with the composability of the existing APIs - this is the kind of thing I do all the time in my native code - but I certainly wouldn't refuse a helper function.

@vstinner
Copy link
Author

vstinner commented Jan 20, 2025

Why not PyImport_ImportModuleAttr[String]?

I'm fine with either names. I have no preference.

@encukou
Copy link
Collaborator

encukou commented Jan 24, 2025

+1 to PyImport_ImportModuleAttr[String].

Should we make these inline static and, as the PR already does, document that they're only combining PyImport_Import and PyObject_GetAttr (plus conversions to PyUnicode)?

This would make sense if done consistently for a larger subset of the API. No point in doing it for two functions -- that would just be a weird special case to document.

+1 for adding these.

@vstinner vstinner changed the title Add PyImport_GetModuleAttrString(mod_name, attr_name) helper function Add PyImport_ImportModuleAttr(mod_name, attr_name) helper function Jan 24, 2025
@vstinner
Copy link
Author

Ok, I renamed the functions to PyImport_ImportModuleAttr[String]().

I would prefer a regular function rather than a static inline function. Static inline functions cannot be used in Rust or C projects only loading symbols such as the vim text editor.

@vstinner
Copy link
Author

Ping @mdboom and @serhiy-storchaka.

@serhiy-storchaka
Copy link

I am the one who introduced these functions in python/cpython#93742. Currently they are used 40 times in the CPython code, this is almost a half of uses of other PyImport functions combined. So we can suppose that they can be useful in third-party code.

On other hand, this is not a fundamental C API. It can be easily implemented using existing C API. Large projects perhaps already implemented a helper for this, and small projects only need it one or two times.

There is also one question which I avoided while it was merely a private helper. This function is not equivalent to Python code from mod import attr, but rather to import mod; mod.attr. The difference is that, unlike Python's "from ... import", it does not use the "from" list. Should it? Using the "from" list will significantly complicate the code, but there are subtle differences (mainly in the raised exception if no attribute exists) between using and not using the "from" list. What exception do we want, ImportError or AttributeError? This should be specified for the public API.

There is also a great inconsistency in naming conventions of PyImport functions that take Python or C string arguments: PyImport_Import/PyImport_ImportModule, PyImport_ImportModuleLevelObject/PyImport_ImportModuleLevel, PyImport_AddModuleObject/PyImport_AddModuleRef, and now PyImport_ImportModuleAttr/PyImport_ImportModuleAttrString, but it cannot be helped.

@vstinner
Copy link
Author

vstinner commented Jan 26, 2025

@serhiy-storchaka:

What exception do we want, ImportError or AttributeError?

IMO it's convenient to get an AttributeError if the attribute doesn't exist.

@serhiy-storchaka:

This should be specified for the public API.

What do you think of the current documentation?

For example, it can raise ImportError if the module is not found, and AttributeError if the attribute doesn't exist.

@serhiy-storchaka
Copy link

The documentation does it right. I asked to be sure that this is an intended behavior and not an artifact of the current implementation. It will be difficult to change after release.

@vstinner
Copy link
Author

The C API Working Group approved the PyImport_ImportModuleAttr() API. Thanks!

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

No branches or pull requests

4 participants