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

Inconsistent behaviour in wildcard import #7669

Closed
1 task done
vepadulano opened this issue Mar 24, 2021 · 6 comments · Fixed by #14588
Closed
1 task done

Inconsistent behaviour in wildcard import #7669

vepadulano opened this issue Mar 24, 2021 · 6 comments · Fixed by #14588

Comments

@vepadulano
Copy link
Member

  • Checked for duplicates

Describe the bug

Some classes are imported in the Python session namespace when wildcard import is used, while others aren't:

vpadulan@fedorathinkpad-T550 [~]: python
Python 3.8.7 (default, Jan 20 2021, 00:00:00) 
[GCC 10.2.1 20201125 (Red Hat 10.2.1-9)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from ROOT import *
>>> RDataFrame
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'RDataFrame' is not defined
>>> TPyDispatcher
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'TPyDispatcher' is not defined
>>> TGHorizontalFrame
<class cppyy.gbl.TGHorizontalFrame at 0x55f59c990900>
>>> RDF
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'RDF' is not defined
>>> TTree
<class cppyy.gbl.TTree at 0x55f59cac9650>
>>> TTreeReader
<class cppyy.gbl.TTreeReader at 0x55f59ce32910>
>>> 

Expected behavior

We should be able to tell more consistenty which names are available after wildcard import, it is handled here .

To Reproduce

from ROOT import * in a python session

Setup

ROOT master@34fc1dc
Fedora 32
built from source

Additional context

This issue stems from #7436

@vepadulano vepadulano self-assigned this Mar 24, 2021
@vepadulano vepadulano changed the title Inconsistent behaviour in wildcard import [PyROOT] Inconsistent behaviour in wildcard import Mar 24, 2021
@vepadulano vepadulano changed the title [PyROOT] Inconsistent behaviour in wildcard import Inconsistent behaviour in wildcard import Mar 24, 2021
@guitargeek
Copy link
Contributor

guitargeek commented Dec 18, 2023

Hi @vepadulano, sorry for the very late reply 😛

I think doing from ROOT import * is quite a bad idea because of the dynamic nature of our Python bindings.

In fact, it doesn't even work when I try it with ROOT master (it just takes forever, the execution never finishes).

So in that sense, we also don't have any inconsistent behavior anymore :)

In fact, maybe we can just make it explicit that this is not supported by raising an exception here?
https://github.com/root-project/root/blob/master/bindings/pyroot/pythonizations/python/ROOT/_facade.py#L182

What do you think?

@vepadulano
Copy link
Member Author

I think doing from ROOT import * is quite a bad idea because of the dynamic nature of our Python bindings.

Indeed it is quite a bad idea in general and I would really like to avoid it 😄 https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

In fact, it doesn't even work when I try it with ROOT master (it just takes forever, the execution never finishes).

This is a bug unfortunately 🥲

What do you think?

Raising the exception is ok, but will break (probably many) existing applications. I am not saying it is a bad thing per se, but maybe we want to soft-break it first with a warning and then break it in a future ROOT release. Need to think about it a bit more, because this will also change whenever we get rid of the facade. But in principle I agree it is the correct way forward

@dpiparo
Copy link
Member

dpiparo commented Feb 3, 2024

We should discuss this matter further. Perhas a soft-break first, à la Python2 deprecation, might be a good starting point.

@guitargeek
Copy link
Contributor

guitargeek commented Feb 5, 2024

A soft-break would require the effort of fixing the wild card import just to drop it afterwards....

There is a structural problem with Python 3.11 that prevents the wildcard import with the lazy lookup to work. Upstream CPyCppyy dropped that feature even:
wlav/CPyCppyy@64fd890
Quote:

As of py3.11, there is no longer a lookup function pointer in the dict object to replace. Since this feature is not widely advertised, it's simply droped

The dropped SetCppLazyLookup() function is exactly the one we used for from ROOT import *.
https://github.com/root-project/root/blob/master/bindings/pyroot/pythonizations/python/ROOT/_facade.py#L162

@vepadulano, given this new information that it will never work with Python 3.11 onward, maybe we should remove the code path for import ROOT *?

@guitargeek
Copy link
Contributor

A soft-break with deprecation warning could still be useful for users of Python 3.8, 3.9 or 3.10, but I doubt that it's worth it, given that no user even noticed that from ROOT import * stopped working with Python 3.11.

guitargeek added a commit to guitargeek/root that referenced this issue Feb 5, 2024
Wild card imports like `from ROOT import *` stopped working with Python
3.11 given some changes in the CPython API.

For that reason, upstream `cppyy` also dropped support for lazy lookups,
which was the feature that enabled wildcard imports:
wlav/CPyCppyy@64fd890#diff-6160c0eb004dabeedaeb58d804a7ecd3e563d9379e9e7af39623fd38d0bc6a37R352

This commit suggests to document `from ROOT import *` officially as
unsupported and remove the corresponding code path in the ROOT facade.

Closes root-project#7669.
@vepadulano
Copy link
Member Author

I am always wary about dropping support for a feature completely without a deprecation notice, even if that feature was buggy and incomplete. This might be one occasion where we simply can't do otherwise

guitargeek added a commit that referenced this issue Feb 5, 2024
Wild card imports like `from ROOT import *` stopped working with Python
3.11 given some changes in the CPython API.

For that reason, upstream `cppyy` also dropped support for lazy lookups,
which was the feature that enabled wildcard imports:
wlav/CPyCppyy@64fd890#diff-6160c0eb004dabeedaeb58d804a7ecd3e563d9379e9e7af39623fd38d0bc6a37R352

This commit suggests to document `from ROOT import *` officially as
unsupported and remove the corresponding code path in the ROOT facade.

Closes #7669.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants