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

feat: add _v2 proxy module #1730

Merged
merged 2 commits into from
Sep 26, 2022
Merged

feat: add _v2 proxy module #1730

merged 2 commits into from
Sep 26, 2022

Conversation

agoose77
Copy link
Collaborator

@jpivarski how do you feel about adding a warning in the future that this is deprecated?

@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #1730 (ad32aab) into main (e692946) will increase coverage by 1.02%.
The diff coverage is n/a.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/__init__.py 97.05% <ø> (ø)
src/awkward/_broadcasting.py 93.41% <ø> (ø)
src/awkward/_connect/avro.py 87.17% <ø> (ø)
src/awkward/_connect/cling.py 24.90% <ø> (ø)
src/awkward/_connect/cuda/__init__.py 0.00% <ø> (ø)
src/awkward/_connect/jax/__init__.py 90.47% <ø> (ø)
src/awkward/_connect/jax/_reducers.py 76.92% <ø> (ø)
src/awkward/_connect/numba/arrayview.py 97.77% <ø> (ø)
src/awkward/_connect/numba/builder.py 81.60% <ø> (ø)
src/awkward/_connect/numba/layout.py 84.87% <ø> (ø)
... and 311 more

@jpivarski
Copy link
Member

We could go through a normal deprecation cycle with this: add a warning that specifies a release when the awkward._v2 shim will be removed (e.g. 2.1.0) and then actually remove it with that release. Let's not do this right away because some people (like us) set warnings as errors in their test suites, so they'd have to exclude that right away. It would be better to set that warning sometime after 2.0.0 is already out, so that it's a later adjustment.

Another thing to consider is that this implementation is simple; adding a warning would make it more complicated. The warning would have to be invoked if awkward._v2 is accessed, not when it's defined (otherwise, even people who don't use it would see the warning). That would make this _v2 object not strictly a module, but maybe some sort of property of the awkward module. It could perhaps be implemented by adding a __getattr__ function in the awkward module, which I think is a Python 3.7+ feature. After issuing the warning, it could return a dynamically constructed module object, made with types.ModuleType.

... Or, we could ignore it completely. Names that start with underscores are not guaranteed interfaces. That was true during v2 development—we can change the API until the 2.0.0 release—and it would continue to be true afterward, as this awkward._v2 does not have to be guaranteed. The normal deprecation cycle is only required for public interfaces, and awkward._v2 isn't (technically) public.

So we can come back to this and reconsider it later. For now, this is a nice implementation because it's so simple. I was imagining making a submodule and manually putting things into it.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

By the way, I approve this for merging.

@agoose77 agoose77 merged commit ff9f1ff into main Sep 26, 2022
@agoose77 agoose77 deleted the agoose77/feat-v2-shim branch September 26, 2022 16:01
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