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.

Sign up for GitHub

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

Jump to bottom

Use `UpdateAllowUnknown' for non-model related parameter. #4961

Merged
merged 2 commits into from
Oct 23, 2019
Merged

Use `UpdateAllowUnknown' for non-model related parameter. #4961

merged 2 commits into from
Oct 23, 2019

Conversation

trivialfis
Copy link
Member

Model parameter can not pack an additional boolean value due to binary IO
format. This commit deals only with non-model related parameter configuration.

@trivialfis
Copy link
Member Author

trivialfis commented Oct 18, 2019 •
edited
Loading

Small part of #4732 . We really owe a lots of debt ...

@hcho3
Copy link
Collaborator

hcho3 commented Oct 22, 2019

Is there a problem that you see only in the test farm?

@trivialfis
Copy link
Member Author

@hcho3 It should be fixed now. Let's see if the tests pass.

@trivialfis
Copy link
Member Author

trivialfis commented Oct 22, 2019 •
edited
Loading

@hcho3 One thing I didn't think of, is clang-tidy doesn't understand static initialization of dmlc::Parameter. It believes that the entry_map_ in parameter manager is empty so uses of it might trigger a clang-tidy error (false positive).

@trivialfis trivialfis force-pushed the update-parameter branch 2 times, most recently from 84d77f7 to 9b69392 Compare October 22, 2019 11:34
@trivialfis
Copy link
Member Author

@hcho3 Done.

hcho3
hcho3 requested changes Oct 22, 2019
View reviewed changes
Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

LGTM. See my comment about clang-tidy.

tests/ci_build/tidy.py Outdated
@@ -234,11 +239,13 @@ def test_tidy():
parser = argparse.ArgumentParser(description='Run clang-tidy.')
parser.add_argument('--cpp', type=int, default=1)
parser.add_argument('--cuda', type=int, default=1)
parser.add_argument('--bundle', type=int, default=1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure how useful this argument is going to be, since currently, tidy.py doesn't let the user specify the path of google test. Either we should add --gtest-path option or remove --bundle option. My personal preference is to always use the bundled gtest.

Copy link
Member Author

Choose a reason for hiding this comment

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

I installed gtest on my system path so CMake always handles it for me...

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, let's rename the option to --use-system-gtest

tests/cpp/common/test_parameter.cc Show resolved Hide resolved
RAMitchell
RAMitchell approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@RAMitchell RAMitchell left a comment

Choose a reason for hiding this comment

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

Looks good. Does it make sense to override InitAllowUnknown such that xgboost developers can't use it?

@trivialfis
Copy link
Member Author

@RAMitchell Not sure about this. We can discuss it after having model parameter (these are runtime parameters).

@trivialfis
Use `UpdateAllowUnknown' for non-model related parameter.
befc551 
Model parameter can not pack an additional boolean value due to binary IO
format.  This commit deals only with non-model related parameter configuration.

* Add tidy command line arg for use-dmlc-gtest.
@trivialfis
Copy link
Member Author

@hcho3 I added a use-dmlc-gtest option and set it to default. The naming is consistent with our CMake parameter.

@trivialfis
Copy link
Member Author

Now I see the problem on Jenkins...

@hcho3
Copy link
Collaborator

hcho3 commented Oct 23, 2019 •
edited
Loading

@trivialfis The convention is to use bundled gtest for all CI tasks.

@trivialfis
Copy link
Member Author

@hcho3 Done.

@trivialfis trivialfis merged commit ac457c5 into dmlc:master Oct 23, 2019
@trivialfis trivialfis deleted the update-parameter branch October 23, 2019 09:50
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hcho3 hcho3 hcho3 approved these changes

@RAMitchell RAMitchell RAMitchell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@trivialfis @hcho3 @RAMitchell