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

Increase timeout for object manager valgrind tests #4027

Merged
merged 4 commits into from
Feb 14, 2019

Conversation

stephanie-wang
Copy link
Contributor

What do these changes do?

Since the inlined objects PR (#3756) was merged, one of the object manager tests was failing on valgrind due to a timing issue. This increases the timeout when running in valgrind.

This also removes an unnecessary data copy when retrieving the inline object data.

Related issue number

Closes #3979.

@@ -342,27 +345,27 @@ class TestObjectManager : public TestObjectManagerBase {
case 0: {
// Ensure timeout_ms = 0 is handled correctly.
// Out of 5 objects, we expect 3 ready objects and 2 remaining objects.
TestWait(600, 5, 3, /*timeout_ms=*/0, false, false);
TestWait(100, 5, 3, /*timeout_ms=*/0, false, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is decreasing the object size intentional here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to put it back under the inline objects limit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, shouldn't we test both?

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

Copy link
Contributor

@guoyuhong guoyuhong left a comment

Choose a reason for hiding this comment

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

Thanks for fix this! I left a minor comment.

# Use timeout=1000ms for the Wait tests.
$CORE_DIR/src/ray/object_manager/object_manager_test $STORE_EXEC 1000
# Run tests again with inlined objects.
$CORE_DIR/src/ray/object_manager/object_manager_test $STORE_EXEC 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use the same parameters for the same test? Is true missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thank you! I added it.

@AmplabJenkins
Copy link

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

@pcmoritz
Copy link
Contributor

Jenkins retest this please

@AmplabJenkins
Copy link

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

@pcmoritz pcmoritz merged commit fd5b58a into ray-project:master Feb 14, 2019
@pcmoritz pcmoritz deleted the fix-om-valgrind branch February 14, 2019 02:29
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.

5 participants