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

refactor: make _v2 facade a static module. #1758

Merged
merged 5 commits into from
Sep 30, 2022

Conversation

jpivarski
Copy link
Member

Instead of making awkward._v2 be referentially the same as awkward, inserted into sys.modules by hand (which does strange things to the __package__ names of all submodules, described here: #1757 (comment)), I made a file named src/awkward/_v2.py and filled it with what the old _v2 had.

Now awkward._v2 is a plain old boring submodule, referentially distinct from awkward, and submodule __package__ names are not changed. It also has exactly the contents that the old awkward._v2 had (not a superset of them), and it gives us a place where we could add a deprecation warning, if we ever want to. The direct imports can be replaced with a module-level __getitem__ that warns before it returns what was requested, like the module-level __getitem__ in Uproot (here).

@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Merging #1758 (c7c14ae) into main (084fa4b) will increase coverage by 0.54%.
The diff coverage is 57.22%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_connect/avro.py 87.17% <0.00%> (ø)
src/awkward/_connect/cuda/__init__.py 0.00% <0.00%> (ø)
src/awkward/_connect/numexpr.py 89.85% <0.00%> (ø)
src/awkward/_connect/rdataframe/from_rdataframe.py 0.00% <0.00%> (ø)
src/awkward/_connect/rdataframe/to_rdataframe.py 0.00% <0.00%> (ø)
src/awkward/behaviors/mixins.py 97.50% <0.00%> (ø)
src/awkward/operations/ak_from_cupy.py 50.00% <0.00%> (ø)
src/awkward/operations/ak_from_rdataframe.py 42.85% <0.00%> (ø)
src/awkward/operations/ak_to_cupy.py 33.33% <0.00%> (ø)
src/awkward/operations/ak_to_rdataframe.py 18.18% <0.00%> (ø)
... and 153 more

@agoose77
Copy link
Collaborator

The only downside of this approach is that from awkward._v2.contents import ListArray will no longer work. That said, I'm not sure that we can easily have both things without e.g. having two versions of Awkward (e.g. awkward._v2.contents.ListArray is not awkward.contents.ListArray). So it comes down to what we prefer.

I have a few more ideas on this, which I'm just trying out :)

@jpivarski
Copy link
Member Author

I'll wait to see your idea. (Leaving this PR open.)

@agoose77
Copy link
Collaborator

agoose77 commented Sep 30, 2022

I've spent some time digging, and I've realised that my understanding of what's happening with respect to the renamed modules is not the full picture. It turns out that the importlib docs aren't one-to-one on what CPython actually does (or at least, they're vague), so one should actually look at the bootstrap implementation. Despite being fairly familiar with the internals of importlib, this next part took me by surprise.

What's actually happening is that Python's creating a new module object for imports of awkward._v2.XXX, and then re-writing the name XXX on the original module. This is actually worse than just mangling the names, because it means that we could have multiple copies of modules lying around with half-shared state.

I had not realised that this would happen, because I'd thought that Python took the __package__ of the _v2 module (which would be "awkward". This was an oversight on my part; this, of course, only applies to relative imports.

So, we need to revert #1730, and I think the safest thing to do is to just have a proxy module like you've shown. That will support from awkward import _v2 as ak and import awkward._v2 as ak, but not from awkward._v2.contents import XXX.
We could pre-populate sys.modules to fix this, but we run into the same problem; multiple instances of the same module would exist if a non-pre-populated module were imported. We could pre-populate all "importable" files, ensuring that this last condition is never met. This, however, would be fragile and a non-insignificant amount of code, so I only mention it for posterity.

So, apologies for holding this back, and indeed taking us down this path to begin with. I was hoping that #1730 would give us clean support for aliasing v2, but it rather turned out to be a liability.

@jpivarski
Copy link
Member Author

Okay, this does revert #1730, and I've been testing it against Uproot, though I think my issues were local to my computer. Uproot's tests passed even with the other way of doing _v2.

So I'll enable auto-squash-and-merge.

@jpivarski jpivarski enabled auto-merge (squash) September 30, 2022 19:59
@jpivarski jpivarski merged commit 9a3620d into main Sep 30, 2022
@jpivarski jpivarski deleted the jpivarski/make-v2-facade-a-static-module branch September 30, 2022 20:16
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.

2 participants