Skip to content

ENH Break cyclic references in Histogram GBRT - #18334

Merged
thomasjpfan merged 5 commits into
scikit-learn:masterfrom
ogrisel:hgbrt-break-cyclick-refs
Sep 3, 2020
Merged

ENH Break cyclic references in Histogram GBRT#18334
thomasjpfan merged 5 commits into
scikit-learn:masterfrom
ogrisel:hgbrt-break-cyclick-refs

Conversation

@ogrisel

@ogrisel ogrisel commented Sep 3, 2020

Copy link
Copy Markdown
Member

Resolves #18152
This is an alternative to #18163 and #18242

This is a minimal fix to:

For the reproducer snippet of #18242:

from sklearn.datasets import make_classification
from sklearn.experimental import enable_hist_gradient_boosting
from sklearn.ensemble import HistGradientBoostingClassifier
from memory_profiler import memory_usage

X, y = make_classification(n_classes=2,
                           n_samples=10_000,
                           n_features=400,
                           random_state=0)

hgb = HistGradientBoostingClassifier(
    max_iter=100,
    max_leaf_nodes=127,
    learning_rate=.1,
    random_state=0,
    verbose=1,
)

mems = memory_usage((hgb.fit, (X, y)))
print(f"{max(mems):.2f}, {max(mems) - min(mems):.2f} MB")

I get:

Fit 100 trees in 36.711 s, (12700 total leaves)
Time spent computing histograms: 28.150s
Time spent finding best splits:  5.183s
Time spent applying splits:      1.046s
Time spent predicting:           0.016s
282.35, 144.09 MB

(instead of 6GB+ on master).

@thomasjpfan

thomasjpfan commented Sep 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Hm I am getting 983.82, 790.27 MB with this PR.

@ogrisel

ogrisel commented Sep 3, 2020

Copy link
Copy Markdown
Member Author

Hm I am getting 983.82, 790.27 MB with this PR.

That's weird... maybe my Python GC is smarter than yours? I am running Python 3.8.3.

@ogrisel ogrisel mentioned this pull request Sep 3, 2020
Closed
@ogrisel

ogrisel commented Sep 3, 2020

Copy link
Copy Markdown
Member Author

Anyways I am fine with closing this in favor of the manual memory management in #18242 if it ensures a more reliable memory usage. I also updated #18242 to remove the useless attributes that introduced the cyclic refs as clean-up.

@NicolasHug

NicolasHug commented Sep 3, 2020

Copy link
Copy Markdown
Member

Why do you say this PR is less reliable @ogrisel?

@ogrisel

ogrisel commented Sep 3, 2020

Copy link
Copy Markdown
Member Author

I re-ran the snippet several times (restarting Python each time) and I consistently get ~140 MB increments instead of the 790.27 MB observed by @thomasjpfan on his machine.

@ogrisel

ogrisel commented Sep 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Why do you say this PR is less reliable @ogrisel?

@thomasjpfan still observes a 790.27 MB increment with this PR on his machine so the GC does not seem to free memory as fast as needed. Therefore, explicit memory management as done in #18242 might still be useful for some users.

@NicolasHug

NicolasHug commented Sep 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

I too consistently observe about 140MB on this PR with Python 3.8.2, on 2 different machines. (1, 4, and 16 threads. Not that it should matter anyway.)

@thomasjpfan can you double check the benchmark you ran make sure you used the same snippet above?

NicolasHug
NicolasHug approved these changes Sep 3, 2020
View reviewed changes

@NicolasHug NicolasHug left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ogrisel

I have a preference for this solution considering its simplicity and it because of its "unitaryness" (brain is frozen, can't think of a word)

We can still use np.empty and initialize histograms in parallel. However I'd rather leave that for a different PR. I feel like #18242 does too many things at once and it's hard to determine the effect of each change.

My only concern here is: how do we prevent cyclic refs from sneaking in in the future? I guess we should write a memory benchmark in asv?

Comment thread sklearn/ensemble/_hist_gradient_boosting/grower.py Outdated
@ogrisel

ogrisel commented Sep 3, 2020

Copy link
Copy Markdown
Member Author

We can still use np.empty and initialize histograms in parallel. However I'd rather leave that for a different PR.

I agree. It was in the back of my head but I wanted to wait a bit before adding it.

@thomasjpfan what's your feeling w.r.t. the need to introduce the HistogramPool?

@NicolasHug

NicolasHug commented Sep 3, 2020

Copy link
Copy Markdown
Member

I think we should:

  1. merge this. It's a net improvement, irrespective of benchmark results: it removes useless attributes and can be considered pure maintenance.
  2. use np.empty and initialize histograms in parallel in another PR, and evaluate the speed improvement
  3. Consider adding the pool then, in yet another PR.

If 1 does its job then I doubt 3 would add a significant memory improvement but it might add a small speed improvement, since it will remove the need from allocating memory each time.

@thomasjpfan

thomasjpfan commented Sep 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

@thomasjpfan what's your feeling w.r.t. the need to introduce the HistogramPool?

Let's merge this one first.

  • On linux with 3.8, I get 245.16, 131.90 MB.
  • On linux with 3.7, I get 256.35, 140.72 MB.
  • On OSX with 3.8, memory profiler does not work with 3.8 yet on osx. edit: When I look at the process with htop I see the memory go to ~1GB. which is consistent with OSX with 3.7.
  • On OSX with 3.7, I get 983.82, 790.27 MB

@ogrisel

ogrisel commented Sep 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Thanks @thomasjpfan for the investigation. Let me add a what's new entry to this PR if you want to merge it as it is. However the results on macOS makes me think that we will need to add the HistogramPool anyway.

@ogrisel

ogrisel commented Sep 3, 2020

Copy link
Copy Markdown
Member Author

Done. Feel free to merge and then rework #18242 on top of this...

NicolasHug
NicolasHug reviewed Sep 3, 2020
View reviewed changes
Comment thread sklearn/ensemble/_hist_gradient_boosting/grower.py Outdated
thomasjpfan
thomasjpfan approved these changes Sep 3, 2020
View reviewed changes

@thomasjpfan thomasjpfan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan changed the title Break cyclick references in Histogram GBRT ENH Break cyclic references in Histogram GBRT Sep 3, 2020
@thomasjpfan
thomasjpfan merged commit e325f16 into scikit-learn:master Sep 3, 2020
@ogrisel
ogrisel deleted the hgbrt-break-cyclick-refs branch September 3, 2020 22:35
@ogrisel ogrisel mentioned this pull request Sep 4, 2020
Closed
@lorentzenchr lorentzenchr mentioned this pull request Sep 5, 2020
Closed
@ogrisel ogrisel mentioned this pull request Sep 11, 2020
Open
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@ogrisel @NicolasHug @jayzed82
ENH Break cyclic references in Histogram GBRT (scikit-learn#18334)
1883041 
Co-authored-by: Nicolas Hug <[email protected]>
@thomasjpfan thomasjpfan mentioned this pull request Nov 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes
+1 more reviewer
@NicolasHug NicolasHug NicolasHug approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module:ensemble

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

High memory usage in HistGradientBoostingClassifier

3 participants

@ogrisel @thomasjpfan @NicolasHug