Skip to content

Implement hasattr(), str() and repr(). - #8442

Merged
sklam merged 10 commits into
numba:mainfrom
stuartarchibald:wip/hasattr_str_repr
Jan 17, 2023
Merged

sklam merged 10 commits into
numba:mainfrom
stuartarchibald:wip/hasattr_str_repr

Conversation

@stuartarchibald

Copy link
Copy Markdown
Contributor

This patch:

  • Implements the hasattr() builtin.
  • Reimplements the str() builtin to behave more like Python in that __str__ and repr are used internally.
  • Implements repr through the use of __repr__.
  • Fixes up hash to check the result of a __hash__ method before returning and adds some more hash implementations.
  • Refactors a common "create a dummy type" method into the standard Numba numba.tests.support.TestCase.
  • Updates the LiteralPropagation compilation pass to recognised inferred constants from hasattr calls.

This patch:

* Implements the `hasattr()` builtin.
* Reimplements the `str()` builtin to behave more like Python in
  that `__str__` and `repr` are used internally.
* Implements `repr` through the use of `__repr__`.
* Fixes up `hash` to check the result of a `__hash__` method
  before returning and adds some more hash implementations.
* Refactors a common "create a dummy type" method into the
  standard Numba `numba.tests.support.TestCase`.
* Updates the `LiteralPropagation` compilation pass to recognised
  inferred constants from `hasattr` calls.
Comment thread numba/cpython/builtins.py Outdated
Comment on lines +987 to +991
# There's no __str__ or __repr__ defined for this object, go via
# the interpreter.
with objmode(pyrepr='unicode_type'):
pyrepr = _get_py_repr_and_warn(obj)
return pyrepr

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TODO: Need to decide about this! Another option is to raise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one more option is to print something like <object XYZType> so it doesn't need objmode

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

From Numba public meeting discussion on 2023-01-10, the use of objmode is going to be replaced with a simple compiled in string cf. #8442 (comment). This is because users are potentially going to str() or repr() objects to essentially stringify them for use elsewhere and reacquiring the GIL in such a case would be an unnecessary performance penalty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is implemented and tested in b819f01.

@stuartarchibald
stuartarchibald marked this pull request as ready for review September 20, 2022 11:35
@stuartarchibald stuartarchibald added the Effort - long Long size effort needed label Sep 20, 2022
@sklam sklam self-assigned this Sep 20, 2022
@sklam sklam added this to the Numba 0.57 RC milestone Sep 20, 2022

@guilhermeleobas guilhermeleobas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGMT. I only have one concern regarding hasattr. Although the impl. for it seems correct. It might produce false negatives in due to how Numba overloads some functions:

@njit
def test():
    s = set([1, 2, 3])
    return hasattr(s, '__len__'), len(s)

print(test())

Should Numba emit a warning for users to use this feature with caution? Or even an acceptlist of functions we know are safe to use (str/repr/hash). Other functions should be safe once one migrates @overload(func) to @overload_method(type, func)

@stuartarchibald

Copy link
Copy Markdown
Contributor Author

Thanks for the review @guilhermeleobas. Two remarks:

  1. Yes, len needs to defer to obj.__len__. I think that's it though for protocol based function support (ignoring operator.*)? Can you think of any others in the Numba code base at present?
  2. Your comment makes me wonder if there ought to be a concept of "protected" overloads to warn users about overloading an implementation at an unusual level, i.e. warn on overloading len that overload obj.__len__ is the preferred behaviour, that sort of thing.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Oct 4, 2022
@guilhermeleobas

Copy link
Copy Markdown
Contributor

I searched for __ on https://docs.python.org/3/library/functions.html and these are the functions I found that has a corresponding dunder method:

  • abs -> __abs__()
  • bin(x) -> __index__(), when x is not an int
  • callable -> __call__()
  • complex -> __complex__() or __float__() or __index__()
  • enumerate -> __next__()
  • float -> __float__() or __index__()
  • hex -> __index__()
  • int -> __int__() or __index__() or __trunc__()
  • iter -> Must support either the iterable or the sequence protocol
  • next -> __next__()
  • reversed -> __reversed__()
  • round -> __round__() for python object

@stuartarchibald

Copy link
Copy Markdown
Contributor Author

I searched for __ on https://docs.python.org/3/library/functions.html and these are the functions I found that has a corresponding dunder method:

Thanks @guilhermeleobas, from memory, I think that Numba has implementations for a reduced set of these, namely:

* `abs` -> `__abs__()`
* `complex` -> `__complex__()` or `__float__()` or `__index__()`
* `enumerate` -> `__next__()`
* `float` -> `__float__()` or `__index__()`
* `int` -> `__int__()` or `__index__()` or `__trunc__()`
* `iter` -> Must support either the iterable or the sequence protocol
* `next` -> `__next__()`
* `round` -> `__round__()` for python object

I'm not sure if it's possible to handle iter, enumerate and next very easily due to the stack based nature of their implementations, the rest, however, should be doable.

@stuartarchibald

Copy link
Copy Markdown
Contributor Author

In the interests of making this feature available for Numba 0.57, what needs to be done to get this patch to a state where it's sufficiently useful to be able to merge it? Is it potentially "just" resolving these questions: #8442 (comment) ?

I don't think that it's going to be possible to handle quite a few of those dunder methods above trivially in this PR. For example abs uses concrete templates which makes it hard to translate to an overload, the constructors like float and int are entangled with the NumPy machinery and others like next are stack based, which makes them reliant on direct lowering (though could go via the overload inliner eventually).

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Oct 24, 2022

@sklam sklam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Besides the minor suggestion to the error message in the untyped_passes.py, the main question is why allow __hash__ to return None instead of raising.

Once false negatives problems and the above suggestions are addressed, i think this patch is safe to be included.

Comment thread numba/core/untyped_passes.py
Comment thread numba/cpython/hashing.py Outdated
Comment thread numba/np/arrayobj.py
Comment thread numba/typed/dictobject.py
Comment thread numba/typed/listobject.py
@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Nov 23, 2022
This fixes the issue noted in review where the `getattr(obj, __hash__)`
call could return a function that when executed returned None, which
is an invalid type for a hash.
@stuartarchibald

Copy link
Copy Markdown
Contributor Author

Thanks for the review @sklam, comments should now be addressed.

@stuartarchibald

stuartarchibald commented Nov 23, 2022 •

Copy link
Copy Markdown
Contributor Author

1c5970d failed in Azure CI on the macos_py37_np118 build with:

======================================================================
FAIL: test_reflect_exception (numba.tests.test_sets.TestSetReflection)
When the function exits with an exception, sets should still be
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/1/s/numba/tests/test_sets.py", line 813, in test_reflect_exception
    self.assertPreciseEqual(s, set([1, 2, 3, 42]))
  File "/Users/runner/miniconda3/envs/azure_ci/lib/python3.7/contextlib.py", line 119, in __exit__
    next(self.gen)
  File "/Users/runner/work/1/s/numba/tests/support.py", line 266, in assertRefCount
    % (old, new, obj))
AssertionError: Refcount changed from 4 to 6 for object: {1, 2, 3, 42}

This doesn't reproduce locally. Rerunning the CI job to see if it's transient.

@stuartarchibald

Copy link
Copy Markdown
Contributor Author

This doesn't reproduce locally. Rerunning the CI job to see if it's transient.

It's not transient.

@stuartarchibald

Copy link
Copy Markdown
Contributor Author

/AzurePipelines run

@azure-pipelines

Copy link
Copy Markdown
Contributor
Azure Pipelines successfully started running 1 pipeline(s).

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jan 3, 2023
@stuartarchibald

Copy link
Copy Markdown
Contributor Author

9ef54ce fixes the transient issue.

@stuartarchibald

stuartarchibald commented Jan 10, 2023 •

Copy link
Copy Markdown
Contributor Author

Need to do: #8442 (comment) Done in b819f01.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jan 10, 2023
Resolved conflicts in:
	numba/tests/support.py
As title. This replaces the objmode fall back with a generic
string referring to an object of the type of the argument.
@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jan 11, 2023
* `f-string`s use `str()` which now doesn't raise at typing time.
* One of the usecases tests relied on objmode so force it as
  `str()` will now always manage to compile in `nopython` mode.
@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jan 17, 2023
@sklam
sklam merged commit 6ba6680 into numba:main Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - long Long size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants