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.

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

Jump to bottom

[PyROOT] Drop support for from ROOT import * #14588

Merged
merged 2 commits into from
Feb 5, 2024
Merged

[PyROOT] Drop support for from ROOT import * #14588

merged 2 commits into from
Feb 5, 2024

Conversation

guitargeek
Copy link
Contributor

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.

@guitargeek
[PyROOT] Drop support for from ROOT import *
e46c662 
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.
@guitargeek guitargeek self-assigned this Feb 5, 2024
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@vepadulano
Copy link
Member

We should probably be extra explicit and provide an empty __all__ list attribute in our __init__.py. In principle, we could also raise an exception every time a user tries from ROOT import * (beware that we have this in our own test suite, so we will see errors)

@guitargeek
Copy link
Contributor Author

guitargeek commented Feb 5, 2024 •
edited
Loading

We should probably be extra explicit and provide an empty __all__ list attribute in our __init__.py

This is already done in in the ROOT facade constructor. Indeed, one can move it directly to __init__.py, but now that you raised that point: why do you think an explicit __all__ is better than not having this attribute at all?

In principle, we could also raise an exception every time a user tries from ROOT import *

That would be nice! Do you know how to do this? Google didn't give an easy solution :(

(beware that we have this in our own test suite, so we will see errors)

I don't think we have it, or do we? I only see it in this test suite where it is deactivated for Python 3:
https://github.com/root-project/roottest/blob/master/python/regression/PyROOT_regressiontests.py#L188

@vepadulano
Copy link
Member

I don't think we have it, or do we? I only see it in this test suite where it is deactivated for Python 3:

Indeed, I remembered we had more, maybe we already got rid of some instances, there is also this file which points at this jira issue related to the argument, so we can also close that one once this is merged 👍

why do you think an explicit all is better than not having this attribute at all?

Explicit is better than implicit, having the attribute means we can also comment on it for readers

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

vepadulano
vepadulano requested changes Feb 5, 2024
View reviewed changes
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Nice thanks! A couple of comments for consideration

bindings/pyroot/pythonizations/python/ROOT/__init__.py Outdated Show resolved Hide resolved
bindings/pyroot/pythonizations/python/ROOT/__init__.py Outdated Show resolved Hide resolved
bindings/pyroot/pythonizations/python/ROOT/__init__.py Outdated Show resolved Hide resolved
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@guitargeek guitargeek requested a review from vepadulano February 5, 2024 14:37
vepadulano
vepadulano approved these changes Feb 5, 2024
View reviewed changes
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 5, 2024

Test Results

    10 files      10 suites   1d 19h 44m 29s ⏱️
 2 498 tests  2 498 ✅ 0 💤 0 ❌
23 879 runs  23 879 ✅ 0 💤 0 ❌

Results for commit 458a1eb.

@guitargeek
[PyROOT] Actively prevent wildcard imports from ROOT and its submodules
f612fda 
With this commit, the user will get an `ImportError` when trying to do
`from ROOT import *` or for example `from ROOT.RooFit import *`.

Before this commit, such commands were succeeding but without actually
importing anything, which might me confusing to the user because the
expect that everything from ROOT gets imported.
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@guitargeek
Copy link
Contributor Author

Just rebased to fix a typo in the commit message

@guitargeek guitargeek merged commit d85256d into root-project:master Feb 5, 2024
13 of 14 checks passed
@guitargeek guitargeek deleted the import_star branch February 5, 2024 18:07
@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2024-02-05T18:08:19.391Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1128 (message):

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Errors:

  • [2024-02-05T18:22:07.122Z] CMake Error at /data/sftnight/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1128 (message):

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2024-02-05T18:59:58.350Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1128 (message):

@phsft-bot
Copy link
Collaborator

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2024-02-05T19:29:12.825Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1128 (message):

@iarspider iarspider mentioned this pull request Feb 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vepadulano vepadulano vepadulano approved these changes

@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner

Assignees

@guitargeek guitargeek

Labels
in:Python Interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent behaviour in wildcard import
3 participants
@guitargeek @phsft-bot @vepadulano