[Stdlib] Add PythonObject.__contains__

[rd4com]

Simple and small implementation,

Improving the 🐍 Python experience !

Example usage:

x = PythonObject([1,2,3])
if 1 in x: 
   print("1 in x")

That method should be replaced by a c-python function if possible.

[JoeLoser]

Great! Do you mind adding some unit tests please?

[rd4com]

🆕 unit tests

👍 yep it is important, should there be more ?

Python throws an error if the object is not iterable,

But mojo gets an internal error: PyObject_GetAttrString failed from it.

Current PR have a debug_assert when we have the error and returns False.

Is it possible to make __contains__ re-raise the same error as python would have thrown ?

[JoeLoser]

@rd4com sorry this one fell off my queue, I will look at this on Monday and your comment about the re-raising an error and see what we can do about that.

[JoeLoser]

!sync

[rd4com]

Edit: see next comments, this one was for previous commit

Yes, it works better, but it is not yet the same error as python:

Unhandled exception caught during execution: 'int' object has no attribute '__contains__'

python

TypeError: argument of type 'int' is not iterable

It could be because we are using __getattr__, see:

Documentation: datamodel

We might need to use something like object.__contains__,

Any idea ?

[rd4com]

edit: implemented in next commits, see next comments

Ok, according to python specification/docs Documentation: membership-test-details :

We could use a new PyObject_HasAttr("__contains__") ,
if it has, we can use it to determine the return value,
else, we can iterate self.__iter__() in the function to determine the return value.
(which would give a chance to Cpython to raise it's iterable error)

CPython also says that it would also try __getitem__() if the object is not iterable.
(but when, since int do not have __iter__, and return an iterable error anyway)

I'll join cpython chat channel if they have one to ask.

The question would be: How to run an operator membership test from cypthon (in, not in)

In addition, in a perfect world, we'd need to return an PythonObject of class bool.

What do you think ?

[rd4com]

Nice 👍 , we partially got this with last commit, it behaves more like python:

in python

class MyClass:
    x = 1

print(123 in MyClass())

TypeError: argument of type 'MyClass' is not iterable

in python

class MyClass:
    x = 1
    def __contains__(self, rhs):
        return True

print(123 in MyClass())

True

in mojo

from python import Python
def main():
    m = Python.import_module("myclass")
    x = m.MyClass()
    print(1 in x)

True

if we remove the __contains__ dunder in myclass.py

Unhandled exception caught during execution: 'MyClass' object is not iterable

 

The difficulty is now to implement the last step of the "in" membership operator test.
(which is using __getitem__)

1. Any suggestion on how to create a test that uses the class created above in mojo?
   (to test classes with import_module or not in test_python_object.mojo)
2. Should we return a PythonObject of class bool?

[JoeLoser]

To clarify, in one of your comments, you're asking about the return type here, correct? E.g. Bool vs PythonObject? I'm fine with Bool as-is. What do you think?

[rd4com]

Yes, the return type of the function 👍

It should be good, Bool can probably implicitly convert to PythonObject if user do chaining.

(but it is something to keep in mind for the future)

[JoeLoser]

Exactly, I had the composability question in mind, and I think we're fine with Bool for now.

[JoeLoser]

Nice 👍 , we partially got this with last commit, it behaves more like python:

in python

class MyClass:
    x = 1

print(123 in MyClass())

TypeError: argument of type 'MyClass' is not iterable

in python

class MyClass:
    x = 1
    def __contains__(self, rhs):
        return True

print(123 in MyClass())

True

in mojo

from python import Python
def main():
    m = Python.import_module("myclass")
    x = m.MyClass()
    print(1 in x)

True

if we remove the __contains__ dunder in myclass.py

Unhandled exception caught during execution: 'MyClass' object is not iterable

 

The difficulty is now to implement the last step of the "in" membership operator test. (which is using __getitem__)

1. Any suggestion on how to create a test that uses the class created above in mojo?
   (to test classes with import_module or not in test_python_object.mojo)
2. Should we return a PythonObject of class bool?

For (1), are you asking about a user-defined type that implements __iter__ but not __contains__? Or are you asking about a type that defines neither __iter__ or __contains, but does define __getitem__? This last one is the case of

Lastly, the old-style iteration protocol is tried: if a class defines getitem(), x in y is True if and only if there is a non-negative integer index i such that x is y[i] or x == y[i], and no lower integer index raises the IndexError exception. (If any other exception is raised, it is as if in raised that exception).

from https://docs.python.org/3/reference/expressions.html#membership-test-details

[rd4com]

Yes, we need to test the whole function (combinations of instances that: __getitem__ __contains__, __iter__).
For that, we need a way to create an instance of a class as PythonObject:
Python.import_module test_classes_for_contains for example.

(In a way that does not pollute the python environment for other tests of PythonObject)

[JoeLoser]

Yes, we need to test the whole function (combinations of instances that: __getitem__ __contains__, __iter__). For that, we need a way to create an instance of a class as PythonObject: Python.import_module test_classes_for_contains for example.

(In a way that does not pollute the python environment for other tests of PythonObject)

One naive approach is just to have the different instances we want to test in their own test file with their own Python environment/CPython interpreter running. It's far from great, but an option.

Could we leverage context managers and have a context managed CPython interpreter environment for each of these to avoid test pollution?

[rd4com]

edit: next commit have tests to assert the environement is clean between test_*.mojo

 

That is a great idea, context managers are good for that 👍

The difficulty is that Python() returns _get_global_python_itf().cpython():

from python import Python
def main():
    A = Python()
    B = Python()
    A.eval("x = 123")

    print(A.evaluate("x")) 
    print(B.evaluate("x"))

The PythonObject returned by import_module is there:

from python import  Python
def main():
    A = Python()
    B = Python()
    m = A.import_module("myclass")
    print(A.import_module("sys").modules.keys()) #myclass
    print(B.import_module("sys").modules.keys()) #myclass

So if we import for testing it would contaminate the environment,
maybe multiples test files is the good option,
what do you think ?

(we should test assert that the environment stays clean with multiple test files too)

Edit: Assertion done, see next commit

[rd4com]

Hello, two more tests to test the testing environment:

• stdlib/test/python/test_testing_evironement_is_clean_A.mojo
  • sets a global variable
  • import my_module.py
  • assert that my_module is in import_module("sys").modules.keys()
• stdlib/test/python/test_testing_evironement_is_clean_B.mojo
  • assert that the global variable raises (not exist)
  • assert that my_module is not in import_module("sys").modules.keys()

For this to work, we need to be sure that the tests are done in that order. (A then B)

What do you think ?

 

In addition:

• The test for the dunder is moved into test_python_object_dunder_contains.mojo
• It imports module_for_test_python_object_dunder_contains.py
  • Class_no_iterable_no_contains
  • Class_no_iterable_but_contains
  • Class_iterable_no_contains

[JoeLoser]

Hello, two more tests to test the testing environment:

• stdlib/test/python/test_testing_evironement_is_clean_A.mojo

  • sets a global variable
  • import my_module.py
  • assert that my_module is in import_module("sys").modules.keys()

• stdlib/test/python/test_testing_evironement_is_clean_B.mojo

  • assert that the global variable raises (not exist)
  • assert that my_module is not in import_module("sys").modules.keys()

For this to work, we need to be sure that the tests are done in that order. (A then B)

What do you think ?

 

In addition:

• The test for the dunder is moved into test_python_object_dunder_contains.mojo

• It imports module_for_test_python_object_dunder_contains.py

  • Class_no_iterable_no_contains
  • Class_no_iterable_but_contains
  • Class_iterable_no_contains

I don't know if we have a way to force the testing order of A and then B — either in lit, or how we run the tests internally in our Bazel environment (CC: @keith here). Perhaps just omit those tests for now but keep the others so we can rebase and land this?

[rd4com]

Hello,
just implemented the changes 👍 :

• ✅ reverted the A B
• ✅ rebase

[JoeLoser]

!sync

[keith]

CC: @keith here). Perhaps just omit those tests for now but keep the others so we can rebase and land this?

probably not something we can enforce, probably best if we can find another way to validate this ordering. or split these tests into separate lit targets so they don't pollute the same global python environment

[modularbot]

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

[modularbot]

Landed in 996fc97! Thank you for your contribution 🎉