Skip to content

Update export_model(): Add split_kind field - #698

Merged
chyunsu3 merged 2 commits into
dmlc:mainlinefrom
chyunsu3:fix_sklearn_export
Sep 1, 2026
Merged

Update export_model(): Add split_kind field#698
chyunsu3 merged 2 commits into
dmlc:mainlinefrom
chyunsu3:fix_sklearn_export

Conversation

@chyunsu3

@chyunsu3 chyunsu3 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes #696

@betatim Can you review? I tested the change using the reproducer script from #696.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.35%. Comparing base (3b2a9a3) to head (9307469).

Files with missing lines Patch % Lines
python/treelite/sklearn/exporter.py 60.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           mainline     #698      +/-   ##
============================================
- Coverage     84.39%   84.35%   -0.04%     
============================================
  Files            75       75              
  Lines          6862     6867       +5     
  Branches        557      557              
============================================
+ Hits           5791     5793       +2     
- Misses         1071     1074       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@betatim betatim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

One thing I'd add is a check that the fields in the imported node dtype match what treelite knows. Because the fields need filling treelite needs to know about them. I think right now it would maybe work but give wrong results silently.

Something like this at the top of exporter.py

# Fields of scikit-learn's private Node struct that this exporter knows how to
# handle. If scikit-learn grows a field outside this set we cannot populate it,
# and since we now allocate with scikit-learn's own NODE_DTYPE the dtype check in
# Tree.__setstate__ will no longer catch it -- the field would silently keep its
# zero value. `left_cat_bitset` is knowingly left zeroed: trees with categorical
# splits are rejected before we allocate.
_KNOWN_NODE_FIELDS = frozenset(
    {
        "left_child",
        "right_child",
        "feature",
        "threshold",
        "left_cat_bitset",
        "impurity",
        "n_node_samples",
        "weighted_n_node_samples",
        "missing_go_to_left",
        "split_kind",
    }
)

and then a check like this in _export_tree:

unknown_fields = sorted(set(NODE_DTYPE.names) - _KNOWN_NODE_FIELDS)
if unknown_fields:
    raise NotImplementedError(
        f"scikit-learn {sklearn_version} stores tree node fields that this "
        f"version of Treelite does not know how to populate: {unknown_fields}. "
        "Exporting would produce a model that predicts incorrectly. "
        "Please upgrade Treelite."
    )

Comment thread python/treelite/sklearn/exporter.py
Comment thread python/treelite/sklearn/exporter.py
Co-authored-by: Tim Head <betatim@gmail.com>
@chyunsu3 chyunsu3 mentioned this pull request Sep 1, 2026
15 tasks
@chyunsu3
chyunsu3 merged commit 18693ca into dmlc:mainline Sep 1, 2026
18 of 20 checks passed
@chyunsu3
chyunsu3 deleted the fix_sklearn_export branch September 1, 2026 23:40
rapids-bot Bot pushed a commit to NVIDIA/cuml that referenced this pull request Sep 3, 2026
Fixes #8507
Fixes #8425
Fixes #8537
Fixes #8548
Requires conda-forge/treelite-feedstock#99

The new release of Treelite incoporates the following bug fixes:
* Update sklearn exporter for scikit-learn 1.10+ (dmlc/treelite#675)
* Fix import for DART in XGBoost 3.3 and 3.4 (dmlc/treelite#688)
* Use new Node dtype in scikit-learn 1.10+ (dmlc/treelite#698)

Authors:
  - Philip Hyunsu Cho (https://github.com/chyunsu3)
  - Tim Head (https://github.com/betatim)
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - James Lamb (https://github.com/jameslamb)
  - Simon Adorf (https://github.com/csadorf)

URL: #8522
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.

Change to scikit-learn tree node type breaks treelite.sklearn.export_model

2 participants