[python-package] Load parameters from model string

[Samsagax]

This fixes #6851 by using the same workaround as when loading the model from a file (#5424).

[jameslamb]

Thanks for taking the time to contribute!

As I just mentioned in #6851 (comment), think the issue goes deeper than just this and that #6101 will be the right fix.

That said, if you want to pursue this PR for just the in-memory string part (not also fixing copy()), then I'm open to it.... but please add a new test covering this behavior.

Otherwise, if you don't have the time/interest in that, we can close this and you can subscribe to #6101.

[Samsagax]

I'll add the requested tests for this based on the minimal example in #6851 . I would like to make this working even if it doesn't fix the copy part since I'll be using the string from memory in my use case (string retrieved from a database elsewhere).

Thanks for taking the time to review it.

[Samsagax]

Ok. Test case added. Should also be useful for #6101

Thanks for having the time to review this.

[jameslamb]

Thanks very much! I'm glad this worked and I think it's a really nice improvement.

I left some suggestions to make the tests stricter. Can you please also move that test case to right after the test on the similar logic for loading from a model file?

https://github.com/microsoft/LightGBM/blob/6437645c4a0c17046be59e4f57d09952e2e0185f/tests/python_package_test/test_engine.py#L1461

[Samsagax]

I think I've done everything as requested. Should I rebase/squash commits into a more recipe-like structure?

[jameslamb]

Thanks, looks great!

As a quick note... I already see the linting failed: https://github.com/microsoft/LightGBM/actions/runs/13703032288/job/38321483346?pr=6852

You can replicate that locally (and auto-fix the formatting issues) like this:

pip install pre-commit
pre-commit run --all-files

No need to squash commits... we squash on merges here, so every pull request = 1 commit on master: https://github.com/microsoft/LightGBM/commits/master/

You should merge in the latest changes whenever updating though... that helps ensure this is tested against the latest state of the target branch. I just did that, by clicking this button:

[Samsagax]

Don't really know why r packaging is failing. But seems to be failing in master too.

[jameslamb]

Don't really know why r packaging is failing. But seems to be failing in master too.

Yes you're right, that job failing is unrelated to your changes. Documented at #6855

[jameslamb]

Thanks very much! I've changed the label to breaking, because this is technically a breaking change from the perspective of anyone who was relying on params passed through the Booster constructor when loading a file, like this:

bst = Booster(model_str=model_str, params={...})

@jmoralez do you have some time to review this too? You're so familiar with these code paths from #5424, I think that'd be really helpful.

I approve of these changes as-is (so nothing else you need to do @Samsagax), but just leaving a "comment" review so my approval won't count towards a merge until we get another approval.

[Samsagax]

Don't know why is failing. But seems the same failure as main.

[jameslamb]

But seems the same failure as main.

Yes, you don't need to do anything else at this point. There are a couple of CI issues here, we're working on them in #6872

[StrikerRUS]

LGTM, thanks!

[StrikerRUS]

/AzurePipelines run

[StrikerRUS]

/AzurePipelines run

To unblock the merge.

I approve of these changes as-is (so nothing else you need to do @Samsagax), but just leaving a "comment" review so my approval won't count towards a merge until we get another approval.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity since it was closed.
To start a new related discussion, open a new issue at https://github.com/lightgbm-org/LightGBM/issues including a reference to this.