feat: replace v1 with v2 (preserve-history) #1721

agoose77 · 2022-09-23T08:40:34Z

@ianna and @jpivarski, I didn't want to squash and merge #1690 into main, as it would destroy all of the v2 history. Whilst that's less catastrophic than in the case of v1, which has been around for much longer, I would rather keep it. That way, future bugs that are just porting errors will be easier to spot, as the history will inform us of this.

I've just refactored the commits in #1690 so that move operations are preserved. You can confirm that this PR does not change the result with

git diff 0c075e1f0d3f35a703d8a36ede0541b297dadae6 6e696b1f91030ffa6871b638d844fbcd76a9b914

If you're both happy with my changes in line with your reviews, then feel free to merge (via rebase and merge, or merge commit (NOT squash!))

codecov · 2022-09-23T08:49:29Z

Codecov Report

Merging #1721 (7827450) into main (e692946) will increase coverage by 1.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted Files	Coverage Δ
src/awkward/__init__.py	`96.87% <ø> (ø)`
src/awkward/_broadcasting.py	`93.38% <ø> (ø)`
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

refactor: (remove v1 highlevel) fix filepaths in tests refactor: (remove v1 highlevel) restore kSliceNone constants for typetracer refactor: (remove v1 highlevel) duplicate v1 `is_sized_iterable` refactor: (remove v1 highlevel) remove v1 imports in v1.__init__ refactor: (remove v1 highlevel) replace references to ak._util with ak._v2._util refactor: (remove v1 highlevel) use new v2 `to_list`in ArrayBuilder

…._v2.__init__

refactor: (remove v1 highlevel) remove _v2 from imports in awkward.__init__ refactor: (remove v1 highlevel) merge v2 and v1 __init__ refactor: (remove v1 highlevel) remove "_v2" component from filepaths fix: catch missing changes since rebase refactor: (remove v1 highlevel) replace `awkward._v2` imports with `awkward` fix: drop v2 style: pre-commit fixes chore: use broadcsting from root refactor: (remove v1 highlevel) remove `__init_old__` that somehow escaped deletion refactor: remove v1 test for record refactor: (forth) remove use of NumpyArray in Forth refactor: fix Avro buffer conversion refactor: replace references to ak.layout for new contents/index refactor: remove Python bindings for contents refactor: remove unused includes refactor: remove content_methods refactor: remove content-related methods from `make_LayoutBuilder` refactor: remove content-related methods from `make_ArrayBuilder` refactor: remove `PersistentSharedPtr` refactor: remove `Iterator` bindings refactor: remove Content Python bindings remove tosjon refactor: remove Content / Type refactor: remove Reducer refactor: remove Reducer.h refactor: remove Form wip: remove forms import refactor: remove Identities refactor: remove Slice refactor: remove dlpack refactor: remove Index refactor: use kernel directly in `UnionArrayBuilder` refactor: remove typed kernel accessors refactor: remove unused kernels refactor: drop unused test data refactor: remove startup & kernel-utils refactor: delete startup include refactor: remove `startup` call refactor: remove `kernel_lib` export refactor: use new/malloc with correct free instead of kernel allocator refactor: add `util::array_deleter` refactor: remove cuda-utils refactor: remove unused code in `utils` refactor: remove unused code in `kernel-utils` refactor: fix kernel location refactor: use `to_list` check instead of isinstance in `builder_fromiter` feat: add Python datetime support to `from_iter` feat: enable `implements` feat: enable `__array_function__` fix: rely on `nplike.to_rectilinear` to type check inputs docs: remove references to v1 classes docs: remove unused / broken references refactor: remove unused symbol export fix: undo bad sed feat: add `deprecate` decorator fix: enable warnings chore: remove unused code chore: remove duplicated constant fix: handle `layout.ArrayBuilder` in `ak.type` test: fix and enable `EmptyArray` test fix: change case to handle low-level ArrayBuilder Removed ToJson and its subclasses. Removed the old/unused from-json functions, leaving the new ones intact. Removed the Python-interfaced (i.e. old) LayoutBuilder. Removed ak.layout in favor of getting ArrayBuilder directly from _ext. refactor: remove redundant if chore: remove old comment chore: remove old code chore: remove commented debug code

ianna

Looks good to me!

Note, I did not go through all deleted tests - I think we can rely on coverage to add more tests if anything is missing.

Please, see a comment inlined. I think, it would be nice to use either long or short form consistently. I think, it can be auto corrected after this PR is merged.

ianna · 2022-09-23T13:08:04Z

src/awkward/contents/emptyarray.py

-            out = ak._v2.index.Index64.empty(0, self._nplike)
-            return ak._v2.contents.numpyarray.NumpyArray(out, None, None, self._nplike)
+            out = ak.index.Index64.empty(0, self._nplike)
+            return ak.contents.numpyarray.NumpyArray(out, None, None, self._nplike)


I guess, eventually we want to use ak.contents.NumpyArray instead of ak.contents.numpyarray.NumpyArray. The same applies to other *Arrays

Right, that would be a good thing to unify (later). I think we'll be able to take all of the short forms, e.g. ak.contents.NumpyArray instead of ak.contents.numpyarray.NumpyArray. The only reason I can think of for the long forms is if there's a cyclic reference and awkward/contents/__init__.py isn't fully evaluated yet, but that can only happen in module-level references.

I think there's something that I don't understand about Python's (or Python 3's) submodule imports because @henryiii said that it should be possible to make awkward/__init__.py's imports order-independent. On another occasion, he said something about Vector being limited because Awkward wasn't using relative imports consistently. I think there's a rule I or we need to learn about that, but then it will be a mechanical or semi-mechanical process to apply it.

(This, by the way, might become its own issue; it doesn't affect PR #1721 dropping v1.)

src/awkward/__init__.py

jpivarski · 2022-09-23T16:51:03Z

I've just refactored the commits in #1690 so that move operations are preserved. You can confirm that this PR does not change the result with
git diff 0c075e1f0d3f35a703d8a36ede0541b297dadae6 6e696b1f91030ffa6871b638d844fbcd76a9b914

I'm unable to verify that these commits are the same, and I can't find 0c075e1 in this branch. (So fatal: bad object 0c075e1f0d3f35a703d8a36ede0541b297dadae6.)

I'm okay with closing #1690 and merging this PR into main, using the non-squashy method you have in mind. (I'm not quite gliterate enough to trust myself to do that, with all of these rebasings. I'd like you to press the button.)

I'd feel a little more secure if there is a https://github.com/scikit-hep/awkward/compare URL that shows the final #1690 commit is equal to one of these commits, and then I can look at the further diffs beyond it that are only here to be certain what happened after that. I just rechecked the last commit of #1690 (6e696b1) and I'm on board up to that point.

jpivarski · 2022-09-23T17:21:11Z

Followed your advice on Slack:

% git checkout agoose77/excise-v1-merge
% git pull 
remote: Enumerating objects: 5, done.
remote: Counting objects: 100% (5/5), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 5 (delta 3), reused 5 (delta 3), pack-reused 0
Unpacking objects: 100% (5/5), 510 bytes | 72.00 KiB/s, done.
From https://github.com/scikit-hep/awkward
   fc1427ab..78274500  agoose77/excise-v1-merge -> origin/agoose77/excise-v1-merge
Updating fc1427ab..78274500
Fast-forward
 src/awkward/__init__.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
% git diff 78274500975cd337cb52c551bf39252075d70680 6e696b1f91030ffa6871b638d844fbcd76a9b914
diff --git a/src/awkward/__init__.py b/src/awkward/__init__.py
index 6c98b8af..27df449f 100644
--- a/src/awkward/__init__.py
+++ b/src/awkward/__init__.py
@@ -41,8 +41,8 @@ import awkward.behaviors.mixins
 import awkward.behaviors.string
 
 behavior = {}
-awkward.behaviors.string.register(behavior)  # noqa: F405
-awkward.behaviors.categorical.register(behavior)  # noqa: F405
+awkward.behaviors.string.register(behavior)  # noqa: F405 pylint: disable=E0602
+awkward.behaviors.categorical.register(behavior)  # noqa: F405 pylint: disable=E0602
 
 # operations
 from awkward.operations import *

This looks good! You have my approval to merge this into main using the method that you have in mind, to preserve the history but collapse the "drop v1" work into one or a few commits on the main branch.

Thanks again!

agoose77 · 2022-09-24T12:48:02Z

So, this PR has HEAD of 7827450. The original and reviewed PR has HEAD 6e696b1. So, the diff between two is

git diff 78274500975cd337cb52c551bf39252075d70680 6e696b1f91030ffa6871b638d844fbcd76a9b914

which yields

diff --git a/src/awkward/__init__.py b/src/awkward/__init__.py
index 6c98b8af..27df449f 100644
--- a/src/awkward/__init__.py
+++ b/src/awkward/__init__.py
@@ -41,8 +41,8 @@ import awkward.behaviors.mixins
 import awkward.behaviors.string

 behavior = {}
-awkward.behaviors.string.register(behavior)  # noqa: F405
-awkward.behaviors.categorical.register(behavior)  # noqa: F405
+awkward.behaviors.string.register(behavior)  # noqa: F405 pylint: disable=E0602
+awkward.behaviors.categorical.register(behavior)  # noqa: F405 pylint: disable=E0602

 # operations
 from awkward.operations import *

Therefore I'm happy to rebase-and-merge.

agoose77 added 2 commits September 23, 2022 09:49

refactor: (remove v1 highlevel) remove v1 tests

c6ca9f0

refactor: (remove v1 highlevel) move tests/v2 into tests

eea3a0e

agoose77 added 4 commits September 23, 2022 09:49

refactor: (remove v1 highlevel) move awkward/_v2 into awkward/

d2a1241

refactor: (remove v1 highlevel) backup awkward.__init__, move awkward…

4f54273

…._v2.__init__

agoose77 force-pushed the agoose77/excise-v1-merge branch from 0c075e1 to fc1427a Compare September 23, 2022 08:50

ianna approved these changes Sep 23, 2022

View reviewed changes

jpivarski reviewed Sep 23, 2022

View reviewed changes

src/awkward/__init__.py Outdated Show resolved Hide resolved

chore: remove pylint disable

7827450

agoose77 merged commit 1c63c02 into main Sep 24, 2022

agoose77 deleted the agoose77/excise-v1-merge branch September 24, 2022 12:48

This was referenced Sep 24, 2022

reference: feat: replace v1 with v2 #1690

Closed

ak._v2.from_iter should recognize Python datetimes/timedeltas #1701

Closed

np.sum fails with awkward arrays in v2 #1612

Closed

Iterating over Records and ak.from_iter #1582

Closed

jpivarski mentioned this pull request Sep 26, 2022

Add a thin awkward._v2 → awkward alias #1729

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace v1 with v2 (preserve-history) #1721

feat: replace v1 with v2 (preserve-history) #1721

agoose77 commented Sep 23, 2022 •

edited

Loading

codecov bot commented Sep 23, 2022 •

edited

Loading

ianna left a comment

ianna Sep 23, 2022

jpivarski Sep 23, 2022

jpivarski commented Sep 23, 2022

jpivarski commented Sep 23, 2022

agoose77 commented Sep 24, 2022

feat: replace v1 with v2 (preserve-history) #1721

feat: replace v1 with v2 (preserve-history) #1721

Conversation

agoose77 commented Sep 23, 2022 • edited Loading

codecov bot commented Sep 23, 2022 • edited Loading

Codecov Report

ianna left a comment

Choose a reason for hiding this comment

ianna Sep 23, 2022

Choose a reason for hiding this comment

jpivarski Sep 23, 2022

Choose a reason for hiding this comment

jpivarski commented Sep 23, 2022

jpivarski commented Sep 23, 2022

agoose77 commented Sep 24, 2022

agoose77 commented Sep 23, 2022 •

edited

Loading

codecov bot commented Sep 23, 2022 •

edited

Loading