[Unicode] support startswith with args, start and end. by dlee992 · Pull Request #8557 · numba/numba

dlee992 · 2022-11-01T08:13:56Z

As it is, support start and end optional arguments for startswith.

Most added code comes from the implementation of endswith, with marginal changes.

PS, I also have aother PR about supporting int(str) based on this one, and I will open that one after merging this.

BTW, I have a question about _nrt=False, I didn't find any document about this, what is it for in the Numba Runtime level? Disable any memory allocation/refct/release?

guilhermeleobas

Thanks for working on this feature @dlee992. I've made a few comments regarding the exception message raised. Other than that, code looks good.

Co-authored-by: Guilherme Leobas <guilhermeleobas@gmail.com>

guilhermeleobas · 2022-11-02T14:15:14Z

I have checked that:

There is no other Pull Request proposing the same change. (If there is
please highlight this and one of the core developers will pick it up.)
The proposed additional NumPy functionality is not deprecated by
NumPy.
Public CI is passing. (With the exception of something unrelated to this PR)
The patch does not touch files in numba.core.
The patch is implemented using the high-level extension API only
(https://numba.readthedocs.io/en/stable/extending/high-level.html).
The patch updates the documentation to reflect the new functionality.
(https://github.com/numba/numba/blob/main/docs/source/reference/numpysupported.rst)
The patch is line wrapped to 80 characters.
Changes to Python source in the patch are flake8 compliant.
The unit tests for the implementation:
- Explicitly check custom error messages are raised appropriately.
- Hit all the branches of the proprosed change (both at type
  inference and run time).
The patch works in a simple manual test in my local checkout.
I cannot break the code in the patch in my local checkout after
trying to do so for a minimum of 10 minutes. The error messages seem
reasonable and there are no "incorrect" results.
I think the code introduced in this change is easy to maintain.
I understand all the code in this PR.
The implementation of the NumPy/CPython function reflects that present in the
NumPy implementation itself (look in the NumPy repository:
https://github.com/numpy/numpy)
- The implementation has references to the region of the NumPy code
  base that provides the same functionality. The references are URLs
  of the form:
  https://github.com/numpy/numpy/tree/<SHA>/path/to/file.py:#<lines>
- The code "looks" reasonably similar to the NumPy/CPython implementation.
- The code uses the same algorithm as the NumPy/CPython implementation, if
  it does not it explains why and this justification is reasonable.
- The type inference part of the Numba implementation restricts the
  types to those declared in the NumPy/CPython docs (or if not present, what
  is accepted in the NumPy/CPython implementation itself).
- The Numba implementation uses the same variable names for
  arguments as in the NumPy/CPython implementation.
- The Numba code has commentary present for anything not-obvious by
  inspection.
- The Numba code accommodates differences that might occur across
  different NumPy versions in a clear fashion and uses
  numba.np.numpy_support.numpy_version to separate the difference
  in implementations.
The unit tests for the implementation:
- Use the same test inputs as the NumPy/CPython unit tests for the same
  functionality. If this is not the case there are comments which
  explain why and this justification is reasonable.
- The NumPy/CPython unit test inputs have reference URLs associated with
  them to track their origin. The references are URLs of the form:
  https://github.com/numpy/numpy/tree/<SHA>/path/to/file.py:#<lines>
- Use unittest style assertions (not pytest style!).
- Check that unsupported/invalid input types are handled as expected
  using the with self.assertRaises() pattern.
- Check that numerically awkward values work as expected e.g.
  NaN, +/-Inf, 0, INT_MAX, INT_MIN etc.
- Check that types which can be converted to arrays (assuming NumPy
  accepts this behaviour) work as expected.
- Check that passing empty versions of containers e.g. empty tuple
  or 0d array works as expected.

…st solely.

dlee992 · 2022-11-03T07:22:30Z

Actually, the API endswith implementation contains similar issues as my previous version for startswith. But I guess we could change that later, after this one is merged. Do one thing at a time...

stuartarchibald

Thanks for the patch. Few minor things to look at else looks good.

stuartarchibald · 2022-11-03T18:08:16Z

+                                            types.NoneType))):
+        raise TypingError('When specified, the arg "end" must be an Integer')
+
+    if isinstance(prefix, (types.Tuple, types.UniTuple)):


Tuple cannot be accepted here unless the contents of the Tuple are all types.StringLiteral. Consider:

@njit def foo(): print('abcde'.startswith(('a', 1))) foo()

it may be easier to just not accept Tuple to force type inference to try with a non-literal types instead.

I prefer to accept a UniTuple of UnicodeType, since it's more consistent with cpython. So I want to add a check to ensure the prefix type as expected. BTW, in your case:

@njit def foo(): print('abcde'.startswith(('a', 1)))

or another case:

prefix = ('a', 1) @njit def foo(prefix): print('abcde'.startswith(prefix))

Both ('a', 1) will be inferred as Tuple(unicode_type, int64), than Tuple containing a StringLiteral.
And in the executable case:

@njit def foo(): print('abcde'.startswith(('a', 'b')))

('a', 'b') will be inferred as UniTuple(unicode_type x 2), so I just have to ensure prefix is a UniTuple with unicode_type dtype. If I'm wrong, please point out with an example. I haven't found a counterexample...

BTW, I found an interesting thing in cpython (which I think we don't have to implement this short-cut strategy):

>>> 'a'.startswith(('a', 5)) True >>> 'a'.startswith((1, 'a')) Traceback (most recent call last): File "/Users/ld/opt/anaconda3/envs/numbaenv/lib/python3.10/code.py", line 90, in runcode exec(code, self.locals) File "<input>", line 1, in <module> TypeError: tuple for startswith must only contain str, not int

Revised, please review this issue again. @stuartarchibald

By default, type inference will try using non-literal types first, and then if that fails, it will try again using literal types. Python source containing tuples of string literals will be inferred as types.Tuple(<types.StringLiteral>, <types.StringLiteral>, ...). e.g.

from numba import njit import numpy as np @njit def foo(): x = ('a', 'b') foo() foo.inspect_types()

You can see the "try non-literal then literal" types behaviour by doing something like:

from numba import njit, types from numba.extending import overload import numpy as np def bar(tup): pass @overload(bar) def ol_bar(tup): print(f"bar:: tup type is {tup}") @njit def foo(): x = ('a', 'b') bar(x) foo()

(ignore the compilation error, just look at what gets printed to stdout).

RE: the 'a'.startswith(('a', 5)), there are implementation details like this in the CPython implementation. They don't need replicating unless there's some proven use case!

…fix more carefully.

stuartarchibald

Thanks for the update @dlee992. I've made a few suggestions about restructuring the type checking with respect to the type of prefix (and a few other minor things). I think once resolved this should be ready to merge.

Note: I'll answer your questions on the diff inline now so as to not get them confused with this review.

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

stuartarchibald

Thanks for the patch and fixes!

support startswith with args, start and end.

5f2cc16

dlee992 requested review from sklam and stuartarchibald as code owners November 1, 2022 08:13

dlee992 changed the title ~~support startswith with args, start and end.~~ [Unicode] support startswith with args, start and end. Nov 1, 2022

dlee992 mentioned this pull request Nov 1, 2022

[Unicode] support int(str, base=10) as cpython does #8558

Closed

esc added the 3 - Ready for Review label Nov 1, 2022

esc assigned guilhermeleobas Nov 1, 2022

esc added this to the Numba 0.57 RC milestone Nov 1, 2022

guilhermeleobas requested changes Nov 1, 2022

View reviewed changes

Comment thread numba/cpython/unicode.py Outdated

Comment thread numba/cpython/unicode.py Outdated

LI Da and others added 3 commits November 2, 2022 10:40

Update numba/cpython/unicode.py

5e65597

Co-authored-by: Guilherme Leobas <guilhermeleobas@gmail.com>

Update numba/cpython/unicode.py

7fa462e

Co-authored-by: Guilherme Leobas <guilhermeleobas@gmail.com>

add testcases to raise exceptions.

4e5960d

dlee992 requested review from guilhermeleobas and removed request for sklam and stuartarchibald November 2, 2022 07:26

guilhermeleobas requested changes Nov 2, 2022

View reviewed changes

Comment thread numba/cpython/unicode.py Outdated

Comment thread numba/tests/test_unicode.py Outdated

guilhermeleobas added 4 - Waiting on author Waiting for author to respond to review Effort - medium Medium size effort needed and removed 3 - Ready for Review labels Nov 2, 2022

address comments: replace argument name and move out the exception te…

76b58c2

…st solely.

dlee992 requested review from guilhermeleobas November 3, 2022 07:20

guilhermeleobas previously approved these changes Nov 3, 2022

View reviewed changes

guilhermeleobas added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Nov 3, 2022

stuartarchibald requested changes Nov 3, 2022

View reviewed changes

stuartarchibald removed the 5 - Ready to merge Review and testing done, is ready to merge label Nov 3, 2022

stuartarchibald added the 4 - Waiting on author Waiting for author to respond to review label Nov 3, 2022

address reviews: rename, make error msg more precise, check tuple pre…

9742576

…fix more carefully.

dlee992 dismissed guilhermeleobas’s stale review via 9742576 November 4, 2022 03:43

stuartarchibald reviewed Nov 7, 2022

View reviewed changes

Comment thread numba/cpython/unicode.py Outdated

Comment thread numba/cpython/unicode.py Outdated

Comment thread numba/cpython/unicode.py Outdated

Comment thread numba/cpython/unicode.py Outdated

Comment thread numba/cpython/unicode.py Outdated

Comment thread numba/cpython/unicode.py

LI Da and others added 5 commits November 7, 2022 20:45

Update numba/cpython/unicode.py

33aaa1a

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

Update numba/cpython/unicode.py

ac5ba28

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

Update numba/cpython/unicode.py

ae74063

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

Update numba/cpython/unicode.py

2216a7e

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

address reviews: clear handling with prefix.

696dd8d

stuartarchibald approved these changes Nov 7, 2022

View reviewed changes

stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review 4 - Waiting on CI Review etc done, waiting for CI to finish labels Nov 7, 2022

stuartarchibald self-assigned this Nov 7, 2022

sklam merged commit 6a69094 into numba:main Nov 7, 2022

dlee992 deleted the complete_startswith branch December 17, 2022 00:56

Conversation

dlee992 commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guilhermeleobas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guilhermeleobas commented Nov 2, 2022

Uh oh!

dlee992 commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartarchibald Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

dlee992 Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlee992 Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dlee992 commented Nov 1, 2022 •

edited

Loading

dlee992 commented Nov 3, 2022 •

edited

Loading

dlee992 Nov 4, 2022 •

edited

Loading

dlee992 Nov 4, 2022 •

edited

Loading