Skip to content

Conversation

@guoyuhong
Copy link
Contributor

@guoyuhong guoyuhong commented Apr 26, 2019

What do these changes do?

  1. Current ray config only has integer items. Some other developers wonder whether we can use other types in ray config and they have to work around to use them. This PR allows other types to be used in ray config.
  2. For id_def.h and ray_config_def.h, some compiler will raise some warning message showed below.
In file included from XXX.cc:12:0:
ray/src/ray/id.h:128:27: warning: extra ‘;’ [-Wpedantic]
 DEFINE_UNIQUE_ID(UniqueID);
                           ^

Related issue number

N/A

Linter

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

Test

Since there is no other types used in ray config now, I test the case of using double, bool, std::string by myself. The code works fine.

@guoyuhong guoyuhong requested a review from raulchen April 26, 2019 05:52
@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-Perf-Integration-PRB/605/
Test PASSed.

@guoyuhong guoyuhong requested a review from jovany-wang April 26, 2019 06:28
@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/13978/
Test FAILed.

Copy link
Contributor

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

Nice change!

Copy link
Contributor

Choose a reason for hiding this comment

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

Another approach to reserve the ; for every RAY_CONFIG () statement is to add a RAY_IGNORE() at the end of this macro like:

#define RAY_CONFIG(type, name, default_value) \
 if (pair.first == #name) {                  \
    std::istringstream stream(pair.second);   \
    stream >> name##_;                        \
    continue;                                 \
  }                                           \
  RAY_IGNORE(nothing)

But I don't know if this way is good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That way looks a bit wired. We can wait for other developers' comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is wired since it's similar to RAY_LOG().

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a slight preference for requiring a ; after the macro also. If you add RAY_IGNORE at the end, then I'd suggest including a comment to explain its purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding RAY_IGNORE looks fine to me as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RAY_IGNORE_EXPR(0) can only be used in functions. It is not allowed to use RAY_IGNORE_EXPR(0) when a class is defined. I used another way to may the ; required.

@guoyuhong guoyuhong requested review from ericl and richardliaw April 28, 2019 09:47
@guoyuhong guoyuhong force-pushed the refactorRayConfig branch from 87eefd6 to 7b15c8a Compare May 7, 2019 15:04
@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-Perf-Integration-PRB/726/
Test PASSed.

@guoyuhong guoyuhong force-pushed the refactorRayConfig branch from 7b15c8a to e682717 Compare May 7, 2019 15:08
@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-Perf-Integration-PRB/727/
Test PASSed.

@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/14135/
Test FAILed.

src/ray/id.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This way seems more weird. I slightly prefer to just remove the semicolons.

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. Since the Macro is only used in id_def.h and ray_config_def.h, it won't break the traditions in most c++ files.

@guoyuhong guoyuhong force-pushed the refactorRayConfig branch from e682717 to fae1b14 Compare May 8, 2019 07:50
@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-Perf-Integration-PRB/735/
Test PASSed.

@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/14142/
Test FAILed.

Copy link
Contributor

@raulchen raulchen left a comment

Choose a reason for hiding this comment

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

LGTM

@guoyuhong guoyuhong merged commit 481bfbd into ray-project:master May 9, 2019
@guoyuhong guoyuhong deleted the refactorRayConfig branch May 9, 2019 03:18
Edilmo added a commit to BonsaiAI/ray that referenced this pull request Feb 10, 2020
This PR contains changes that help with memory issues
ray-project#4701
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