Change resource bookkeeping to account for machine precision. #4533

williamma12 · 2019-04-02T04:03:29Z

What do these changes do?

Changes the internal representation of resource to integers that represent 1/10000 of a unit of each resource to avoid machine precision of doubles

Related issue number

Closes #4503

Linter

I've run scripts/format.sh to lint the changes in this PR.

AmplabJenkins · 2019-04-02T04:06:44Z

Can one of the admins verify this patch?

AmplabJenkins · 2019-04-02T04:37:25Z

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

ericl · 2019-04-03T03:22:46Z

I'm skeptical this is enough to fix the problems. Should we just always `std::round`` resources to some finite level (1/10000 of a unit)?

robertnishihara · 2019-04-03T21:30:16Z

Yes, I discussed this with @williamma12 today. We're going to try to just make the arithmetic exact. This epsilson would have to be put in too many places and is going to be error prone.

AmplabJenkins · 2019-04-07T06:09:31Z

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

AmplabJenkins · 2019-04-07T08:00:20Z

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

robertnishihara · 2019-04-08T06:50:23Z

This fixes some bugs, right? Is there a reliable way to trigger the error (so that we can add a test for it)?

robertnishihara · 2019-04-08T06:54:41Z

How about something like this?

import numpy as np
import ray
ray.init(num_cpus=2, num_gpus=2, resources={"Custom": 2})

@ray.remote
def f():
    return 1

ray.get([f._remote(num_cpus=np.random.uniform()) for _ in range(100)])
ray.get([f._remote(num_gpus=np.random.uniform()) for _ in range(100)])
ray.get([f._remote(resources={"Custom": np.random.uniform()}) for _ in range(100)])

# NOTE: You'll need to try the check below in a loop because available_resources is updated asynchronously.
assert ray.global_state.available_resources() == {'CPU': 2.0, 'Custom': 2.0, 'GPU': 2.0}

williamma12 · 2019-04-09T00:23:45Z

@robertnishihara Thanks! Is the loop check needed even though the ray.get is a blocking call?

@romilbhardwaj Does this fix work with #4555?

AmplabJenkins · 2019-04-09T02:50:05Z

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

AmplabJenkins · 2019-04-09T18:26:22Z

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

AmplabJenkins · 2019-04-10T04:49:33Z

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

python/ray/tests/test_basic.py

robertnishihara · 2019-04-10T06:21:40Z

src/ray/raylet/scheduling_resources.h

I think this should probably be a separate class (which has an int fieldor evenuint8_t` since it only goes from 0 to 1000 or 10000). That will prevent accidental usage errors. It will mean that you need to add some methods for doing addition and subtraction and maybe checking if it's equal to 1.

Actually let's use int instead of uint8_t. That way we can check that it never goes below 0 and stays in the appropriate range.

I don't think we can set the upper bound to 1*conversion_factor_ since the NodeManager.local_available_resources_ uses ResourceIdSet, which uses ResourceSets and does not differentiate between whole and fractional resource, and, as a result, FractionalResourceQuantity ends up representing resources beyond 1.

src/ray/raylet/scheduling_resources.cc

AmplabJenkins · 2019-04-14T21:58:38Z

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

AmplabJenkins · 2019-04-14T22:57:29Z

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

AmplabJenkins · 2019-04-14T23:28:51Z

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

AmplabJenkins · 2019-04-15T01:31:01Z

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

AmplabJenkins · 2019-04-15T01:42:14Z

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

AmplabJenkins · 2019-04-15T01:52:21Z

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

src/ray/raylet/scheduling_resources.h

robertnishihara · 2019-04-17T00:06:43Z

src/ray/raylet/scheduling_resources.h

Why do we need this constructor? Can we get rid of it?

We have to have an empty constructor because when we add values to resource_capacity_ in ResourceSets, std::unordered_map makes use of the default constructor of zero arguments.

https://github.com/ray-project/ray/blob/1592ead002086768b8ec6ea609bd9a625a1dc08d/src/ray/raylet/scheduling_resources.cc#L99-L104

robertnishihara · 2019-04-23T06:00:52Z

python/ray/tests/test_basic.py

Isn't this going to be really slow since we're doing ray.get inside of the loop? One option is just to put the assert inside of the remote function so you don't need to check the return value.

AmplabJenkins · 2019-04-23T06:09:38Z

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

AmplabJenkins · 2019-04-23T07:49:45Z

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

AmplabJenkins · 2019-04-23T08:10:15Z

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

AmplabJenkins · 2019-04-23T09:03:42Z

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

AmplabJenkins · 2019-04-23T18:27:45Z

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

This PR contains changes that help with memory issues ray-project#4533

williamma12 changed the title ~~Issues/4503~~ Account for machine precision in resource allocation Apr 2, 2019

williamma12 mentioned this pull request Apr 2, 2019

Test failure in test_actor.py::test_resource_assignment on Travis. #4503

Closed

williamma12 changed the title ~~Account for machine precision in resource allocation~~ Change internal representation of resource to account for machine precision Apr 7, 2019

williamma12 force-pushed the issues/4503 branch from 5e3ddc0 to 76d74c2 Compare April 7, 2019 05:40

williamma12 force-pushed the issues/4503 branch from e049ebf to 35d9f44 Compare April 10, 2019 02:13

robertnishihara reviewed Apr 10, 2019

View reviewed changes

williamma12 force-pushed the issues/4503 branch 2 times, most recently from db53ccb to 06214f2 Compare April 14, 2019 21:10

robertnishihara reviewed Apr 17, 2019

View reviewed changes

src/ray/raylet/scheduling_resources.h Outdated Show resolved Hide resolved

robertnishihara reviewed Apr 17, 2019

View reviewed changes

src/ray/raylet/scheduling_resources.h Outdated Show resolved Hide resolved

robertnishihara reviewed Apr 17, 2019

View reviewed changes

src/ray/raylet/scheduling_resources.h Outdated Show resolved Hide resolved

robertnishihara reviewed Apr 17, 2019

View reviewed changes

williamma12 added 17 commits April 22, 2019 22:57

linting and fix test

2e712f1

linting

2f25c12

called GetResourceMap instead of GetResourceAmountMap

bf1eae4

Remove print statements

bad97fd

resolve comments

3e4b352

linting

c101484

Change test to loop 10 sec instead of 10 min

df424da

Resolve comments

1d0140a

Try undoing scheduling_policy.cc change

3aa513d

Clean up code

67021d6

linting

17c65ad

revert unnecessary change

c72d3c7

Refine comment to make it clearer

cf1140b

Passing zero resource deletion test

449ace4

Fix subtractResources

411f7f3

Change empty resourceSet to print {}

38c467e

linting

143550f

robertnishihara reviewed Apr 23, 2019

View reviewed changes

linting

c239322

williamma12 force-pushed the issues/4503 branch from 79279fa to c239322 Compare April 23, 2019 06:03

linting and comments

c6253fd

more linting

18c075b

robertnishihara approved these changes Apr 23, 2019

View reviewed changes

robertnishihara merged commit c99e3ca into ray-project:master Apr 23, 2019

Edilmo added a commit to BonsaiAI/ray that referenced this pull request Feb 10, 2020

Cherry picking changes from PR 4533

85da751

This PR contains changes that help with memory issues ray-project#4533

Change resource bookkeeping to account for machine precision. #4533

Change resource bookkeeping to account for machine precision. #4533

Uh oh!

Conversation

williamma12 commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Related issue number

Linter

Uh oh!

AmplabJenkins commented Apr 2, 2019

Uh oh!

AmplabJenkins commented Apr 2, 2019

Uh oh!

ericl commented Apr 3, 2019

Uh oh!

robertnishihara commented Apr 3, 2019

Uh oh!

AmplabJenkins commented Apr 7, 2019

Uh oh!

AmplabJenkins commented Apr 7, 2019

Uh oh!

robertnishihara commented Apr 8, 2019

Uh oh!

robertnishihara commented Apr 8, 2019

Uh oh!

williamma12 commented Apr 9, 2019

Uh oh!

AmplabJenkins commented Apr 9, 2019

Uh oh!

AmplabJenkins commented Apr 9, 2019

Uh oh!

AmplabJenkins commented Apr 10, 2019

Uh oh!

Uh oh!

robertnishihara Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

robertnishihara Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

williamma12 Apr 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AmplabJenkins commented Apr 14, 2019

Uh oh!

AmplabJenkins commented Apr 14, 2019

Uh oh!

AmplabJenkins commented Apr 14, 2019

Uh oh!

AmplabJenkins commented Apr 15, 2019

Uh oh!

AmplabJenkins commented Apr 15, 2019

Uh oh!

AmplabJenkins commented Apr 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertnishihara Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

williamma12 Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

robertnishihara Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

Reviewers

williamma12 commented Apr 2, 2019 •

edited

Loading