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

fix typo #1227

Closed
wants to merge 3 commits into from
Closed

fix typo #1227

wants to merge 3 commits into from

Conversation

martindurant
Copy link
Contributor

No description provided.

@@ -354,7 +354,7 @@ def mergeable(self, other, mergebool):
if isinstance(
other,
(
ak._v2.contents.emptyArray.EmptyArray,
ak._v2.contents.emptyarray.EmptyArray,
Copy link
Contributor Author

@martindurant martindurant Jan 17, 2022

Choose a reason for hiding this comment

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

or should it just be ak._v2.contents.EmptyArray ? Contents module already imports all the things. In this module, there are a number of these, but the others follow the lower-case spelling.

Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be any lower camelCase module names at all.

Using

egrep -r '\b[a-z]+[A-Z]' src/awkward/_v2/**/*.py | egrep -v '\bto' | egrep -v '\bkMax' | egrep -v '\bkSlice'

I found the one you just found and another one in indexedarray.py (in addition to the original), though that's it; the rest of the regex matches are not instances of this copy-paste error.

src/awkward/_v2/_connect/numexpr.py:# def getArguments(names, local_dict=None, global_dict=None):
src/awkward/_v2/_connect/numexpr.py:#     context = numexpr.necompiler.getContext(kwargs, frame_depth=1)
src/awkward/_v2/_connect/numexpr.py:#         numexpr.necompiler._names_cache[expr_key] = numexpr.necompiler.getExprNames(
src/awkward/_v2/_connect/numexpr.py:#     arguments = getArguments(names, local_dict, global_dict)
src/awkward/_v2/_connect/numexpr.py:#     arguments = getArguments(names, local_dict)
src/awkward/_v2/_connect/numpy.py:#         rowsA = len(A)
src/awkward/_v2/_connect/numpy.py:#         colsA = first
src/awkward/_v2/_connect/numpy.py:#         rowsB = len(B)
src/awkward/_v2/_connect/numpy.py:#         colsB = first
src/awkward/_v2/_connect/numpy.py:#         if colsA != rowsB:
src/awkward/_v2/contents/bitmaskedarray.py:                ak._v2.contents.emptyArray.EmptyArray,
src/awkward/_v2/contents/indexedarray.py:                ak._v2.contents.emptyArray.EmptyArray,
src/awkward/_v2/contents/listarray.py:            next_content = asListOffsetArray64._content
src/awkward/_v2/highlevel.py:_dir_pattern = re.compile(r"^[a-zA-Z_]\w*$")
src/awkward/_v2/operations/convert/ak_from_json_schema.py:        "`\\uXXXX`" escape sequences. If an `"enum"` is given, strings are represented
src/awkward/_v2/operations/convert/ak_from_json_schema.py:        if schema.get("minItems") == schema.get("maxItems") != None:  # noqa: E711
src/awkward/_v2/operations/convert/ak_from_json_schema.py:            assert ak._v2._util.isint(schema.get("minItems"))
src/awkward/_v2/operations/convert/ak_from_json_schema.py:            instructions.append(["FixedLengthList", schema.get("minItems")])
src/awkward/_v2/operations/convert/ak_from_json_schema.py:            out = ak._v2.forms.RegularForm(content, size=schema.get("minItems"))
src/awkward/_v2/operations/convert/ak_to_buffers.py:    Dask, but partition numbers can be emulated by prepending a fixed `"partN-"`

As for whether it should be ak._v2.contents.emptyarray.EmptyArray or ak._v2.contents.EmptyArray, I slightly prefer the latter, though I'd make it fit the context of the surrounding code, if relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, in that case my version is fine, right?

Copy link
Member

Choose a reason for hiding this comment

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

Your version is fine, though there's another one just like this in indexedarray.py:

src/awkward/_v2/contents/indexedarray.py:                ak._v2.contents.emptyArray.EmptyArray,

Since you reported the first one in unmaskedarray.py, I figure it's only a matter of time before you hit the one in indexedarray.py, too.

Meanwhile, I'm more worried about the build failing. You made a change that can't affect compilation, and yet every compilation is failing (in pybind11, which was recently updated...).

@jpivarski
Copy link
Member

I'm wondering why all the tests are failing in the build. I'm checking out a fresh copy of the repo and rebuilding it myself.

I'm also running the whole test suite on main: https://dev.azure.com/jpivarski/Scikit-HEP/_build/results?buildId=8717&view=results

@jpivarski
Copy link
Member

I'm also running the whole test suite on main: https://dev.azure.com/jpivarski/Scikit-HEP/_build/results?buildId=8717&view=results

The main branch is okay. I wonder what...

Aha! This PR is reverting the old pybind11. That's one thing I don't like about submodules; they don't version control the same way as other files. Since this is a tiny PR, would you mind if I just replace it with a new one that only makes your two intended changes?

@martindurant
Copy link
Contributor Author

would you mind if I just replace it with a new one

Certainly. On Thursday you van tell me when the difference was :)

@jpivarski
Copy link
Member

It's just this (no need to take time out of next Thursday's meeting for it):

image

You had a local git repo of awkward with pybind11 in it from before the submodule was updated, and then even if you git pull on main, everything will be updated except the submodules. When you checked in your changes, you checked in the old submodule, thereby downgrading it. @ioanaif did it earlier this week and I've done it on numerous occasions. The way I avoid it now is to redo a git clone --recursive ... whenever a submodule changes. (I did one just now to make sure that I had a clean slate to test compilation.)

Thus, I don't like submodules, but they're still the best solution we have for C++ header-only dependencies, and they're not too bad if we hardly ever update them (which is the case).

@jpivarski
Copy link
Member

Replaced by #1228.

@jpivarski jpivarski closed this Jan 17, 2022
@martindurant
Copy link
Contributor Author

Reopening just to see if I successfully updated submodules - do not merge.
May I suggest we include the submodule update in a commit hook?

@martindurant
Copy link
Contributor Author

hm, can't reopen - never mind

@jpivarski
Copy link
Member

I could reopen, so that you can test.

But updating a submodule (without simply re-cloning the whole repo) is an annoying process: it involves cd into the submodule, git checkout of the submodule, using its repo's commit numbers, cd back out, and an unqualified git commit. It's a procedure I have to look up every time, so I try to avoid doing it.

Catching accidental changes to submodules in a pre-commit could be a good idea, though they really do change rarely. Soon, we'll be dropping dlpack and will only have two of them: pybind11 and RapidJSON.

@jpivarski jpivarski reopened this Jan 17, 2022
@martindurant
Copy link
Contributor Author

martindurant commented Jan 17, 2022

Something like

git submodule update --recursive --remote --force

(question)

@martindurant
Copy link
Contributor Author

Apparently it's a "no". Closing anyway :)

@jpivarski
Copy link
Member

Maybe, but like I said, I have to look it up each time. (That would be true of anything I do less frequently than once a week.)

Thinking about the pre-commit check to prevent accidental submodule reversions, which is the biggest issue: git sees the submodule as a detached head—I don't think it would be able to distinguish "less up-to-date" from "more up-to-date." It would only see "different." Then, having such a pre-commit could make it hard to legitimately update the submodule.

Unless the issue happens more frequently, it might be better to not create a solution for it that might have other problems. Currently, it's still the case that I'm the "single point of merge bottlenecks," so I can just be more vigilant about checking for that when I do a review. (The reason this hasn't come up before is because previously, I was also the one doing most of the commits as well. Now that we've moved into a phase where there's multiple contributors, but I'm still reviewing everything, I have to be on the watch for stale submodules. We don't yet need to incorporate that into formal review checklists, though, because that would be something for the phase, with multiple reviewers.)

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