Add test: running a driver for twice.

[surehb]

What do these changes do?

Add test for PR: Use different serialization context for each driver..

Related issue number

#2406

[surehb]

@robertnishihara, please take a look. Thanks.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6795/
Test PASSed.

[robertnishihara]

Can you include the full URL instead of just the issue number? This will make it easier to navigate to the issue.

[surehb]

OK.

[robertnishihara]

I'd suggest changing this test to not use Ray tune (since the implementation of Ray tune may change). Instead, I'd consider doing something like the following

driver1 = """
...
def serializer(obj):
    return 1

def deserializer(serialized_obj):
    assert serialized_obj == 1
    return 1

class Foo(object):
    pass

ray.register_custom_serializer(serializer=serializer, deserializer=deserializer)

@ray.remote
def f(x):
    assert x == 1
    return Foo()

for _ in range(10):
    time.sleep(0.1)
    assert ray.get([f.remote() for _ in range(10)]) == 1
"""

In driver2 all of the 1s should be replaced by 2.

I'd make num_cpus=1 so that all tasks run on the same worker.

This is just an example, but I think it isolates the lower-level behavior a little more clearly. What do you think?

Also, please make sure that the test fails without the patch.

[surehb]

I would prefer the existing way since:

1. What this test covers is whether we can run a driver for twice successfully, I believe we will need this kind of test sooner or later;
2. Ray tune is the typical case that used to run into problem, it makes more sense to use it to test the fix for this case;
3. I understand that the current test is more like a E2E test, and what you propose is more like unit test. I agree that it will be better for us to have completed unit test, for example we need to test: type register on worker -> verify driver and other workers; type register on driver -> verify all workers. I can do it later.

[robertnishihara]

Ok, I think having a unit test at some point would be valuable, but I agree that this test makes sense.

[robertnishihara]

How long does this test take to run? If it's quick then maybe we want both.

[surehb]

It runs about 10 seconds, what do you mean by "both"?

[robertnishihara]

I meant "this test" and "the unit test I proposed above."

Anyway, 10 seconds is on the long side. It's ok in this case, but in general we should keep tests as short as possible.

[surehb]

Thank you @robertnishihara!

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6826/
Test FAILed.

[surehb]

@robertnishihara, can you help to merge? Thank you!