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

[Breaking] Remove dmlc::Parameter #293

Merged
merged 1 commit into from
Jul 11, 2021
Merged

Conversation

hcho3
Copy link
Collaborator

@hcho3 hcho3 commented Jul 9, 2021

Extracted from #285.

  • Do not use dmlc/parameter.h.
  • CompilerParam no longer inherits from dmlc::Parameter. Consequently, it now implements a special method ParseFromJSON() that ingests parameters from a JSON string.
  • Now parameters are set with JSON strings.
    Before:
CompilerHandle handle;
TreeliteCompilerCreate("ast_native", &handle);
TreeliteCompilerSetParam(handle, "quantize", "1");
TreeliteCompilerSetParam(handle, "parallel_comp", "100");
TreeliteCompilerSetParam(handle, "native_lib_name", "predictor");

After:

CompilerHandle handle;
const char* params = "{\"quantize\": 1, \"parallel_comp\": 100,"
                     " \"native_lib_name\": \"predictor\"}";
TreeliteCompilerCreateV2("ast_native", params, &handle);
  • Breaking: The C API functions TreeliteCompilerCreate and TreeliteCompilerGenerateCode are renamed to TreeliteCompilerCreateV2 and TreeliteCompilerGenerateCodeV2, respectively, to indicate the different calling signature. In addition, TreeliteCompilerSetParam has been removed.
  • Add C++ tests to test the correctness of ParseFromJSON().
  • Upgrade Google Test framework to 1.11.0 to test the content of exceptions.

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #293 (8bfcbb4) into mainline (5d7d968) will decrease coverage by 0.78%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##             mainline     #293      +/-   ##
==============================================
- Coverage       84.78%   83.99%   -0.79%     
  Complexity         46       46              
==============================================
  Files             101      102       +1     
  Lines            7899     7909      +10     
  Branches           50       50              
==============================================
- Hits             6697     6643      -54     
- Misses           1177     1241      +64     
  Partials           25       25              
Impacted Files Coverage Δ
include/treelite/c_api_error.h 100.00% <ø> (ø)
include/treelite/compiler.h 100.00% <ø> (ø)
include/treelite/compiler_param.h 100.00% <100.00%> (ø)
python/treelite/frontend.py 76.69% <100.00%> (-0.25%) ⬇️
src/c_api/c_api.cc 97.09% <100.00%> (+0.43%) ⬆️
src/compiler/ast_native.cc 96.09% <100.00%> (+0.01%) ⬆️
src/compiler/compiler.cc 96.36% <100.00%> (+23.63%) ⬆️
src/compiler/failsafe.cc 86.82% <100.00%> (+0.12%) ⬆️
tests/cpp/test_compiler_param.cc 100.00% <100.00%> (ø)
python/treelite/contrib/msvc.py 0.00% <0.00%> (-67.15%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d7d968...8bfcbb4. Read the comment docs.

_check_call(_LIB.TreeliteCompilerGenerateCode(
if verbose and _params:
_params['verbose'] = 1
params_json_str = json.dumps(_params) if _params else '{}'
Copy link
Member

Choose a reason for hiding this comment

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

I think JSON handles this automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the None object gets serialized to null.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, the param is optional.

@hcho3 hcho3 merged commit 262f782 into dmlc:mainline Jul 11, 2021
@hcho3 hcho3 deleted the remove_dmlc_parameter branch July 11, 2021 10:22
@hcho3 hcho3 mentioned this pull request Jul 16, 2021
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.

2 participants