Fix old syntax xgboost() results in stack overflow

[tylerjthomas9]

This PR is a patch to fix the issue described here: https://github.com/dmlc/XGBoost.jl/issues/130. I have added a function for the old api with a depreciation warning.

Let me know if I should change anything in the PR.

[ExpandingMan]

This mostly looks ok, but two comments:

• I suppose there was some argument that we should have had this old xgboost method with a deprecation warning when 2.0 was released, but even conceding that, the ship has long sailed. I'm definitely not in favor of adding this back in.
• It's usually not good practice to duplicate code the way it was done with update! here, but admittedly it's hard to see a good alternative in this case. Maybe and _update! method that takes a tuple as an argument?

[tylerjthomas9]

• I suppose there was some argument that we should have had this old xgboost method with a deprecation warning when 2.0 was released, but even conceding that, the ship has long sailed. I'm definitely not in favor of adding this back in.

Removed it!

Now, the old syntax gives the following error:

ERROR: MethodError: no method matching updateone!(::Booster, ::DMatrix, ::Int64; round_number=1, watchlist=Dict{String, DMatrix}("train" => DMatrix(100, 4)))
Closest candidates are:
  updateone!(::Booster, ::DMatrix, ::Any, ::Any; kw...) at ~/XGBoost.jl/src/booster.jl:379
  updateone!(::Booster, ::Any, ::Any, ::Any; kw...) at ~/XGBoost.jl/src/booster.jl:386
  updateone!(::Booster, ::DMatrix; round_number, watchlist, update_feature_names) at ~/XGBoost.jl/src/booster.jl:338

• It's usually not good practice to duplicate code the way it was done with update! here, but admittedly it's hard to see a good alternative in this case. Maybe and _update! method that takes a tuple as an argument?

We can just keep a generic version of update!. How does this look?

function update!(b::Booster, data, a...; num_round::Integer=1, kw...)
    for j ∈ 1:num_round
        round_number = getnrounds(b) + 1
        updateone!(b, data, a...; round_number, kw...)
    end
    b
end

[ExpandingMan]

Now, the old syntax gives the following error:

Yeah, I get it, this is definitely going to piss somebody off. Again, the ship has sailed, so if people really want that somebody other than me can merge it.

This new version of update! might be a little too clever (it might still lead to method ambiguities or stack overflows down the road) but it's definitely an improvement on what we had before, so I'm definitely good with this for now.

I'd like to merge this, but windows is failing and I have no clue why... any ideas?

[tylerjthomas9]

I'd like to merge this, but windows is failing and I have no clue why... any ideas?

I have been trying to figure out what was going on with windows. I thought that it may have been my machine, so its interesting that the runner is also failing with the same exit code. I am getting the same issues on older versions of xgboost.jl + xgboost_jll. Trying to figure out what broke.

[tylerjthomas9]

I am only getting windows issues on Julia v1.8.4. All the tests pass on v1.8.3. I am looking into changes that might've caused this.

[ExpandingMan]

Also #153 was just opened. I suspect there is some bug with Julia 1.8.4 on windows.

I have no access to windows and haven't even used it in years, so there's nothing more I can say about this.

I strongly suspect that whatever is causing this failure has nothing to do with this PR, but I'll still hold off on merging it for a while so at least we can claim that windows was passing last time CI/CD ran.

[ExpandingMan]

Can we re-trigger this somehow? I'd like to see if windows still fails with 1.8.5. @tylerjthomas9 could you maybe make some trivial commit to cause it to re-run?

[ExpandingMan]

Sigh, guess not. This is frustrating, I'd like to merge but I don't want to take responsibility for breaking the windows build, even though it's very likely already broken.

[tylerjthomas9]

Sigh, guess not. This is frustrating, I'd like to merge but I don't want to take responsibility for breaking the windows build, even though it's very likely already broken.

https://discourse.julialang.org/t/issue-with-xgboost-jl-and-libsvm-jl-when-julia-1-8-4/92396/8?u=tylerjthomas9

Tonight, I will try this git-bisect method to pinpoint the commit. I really want it fixed before Julia v1.9 is released, because it'll become hard to justify staying on v1.8.3

[tylerjthomas9]

https://discourse.julialang.org/t/issue-with-xgboost-jl-and-libsvm-jl-when-julia-1-8-4/92396/10?u=tylerjthomas9

My own PR that backported the CSL upgrade is causing the issue. Sorry for that... I guess future versions of julia would've broke anyways. I am trying to figure out why bumping CSL would cause this issue, especially since it is only having issues on windows.

[ExpandingMan]

I am trying to figure out why bumping CSL would cause this issue, especially since it is only having issues on windows.

This probably isn't the forum in which I should share my opinions on windows, so I will refrain.

So, can you confirm that this is a problem with Julia 1.8.4 generally? If so, I will merge this since the failure almost certainly has absolutely nothing to do with XGBoost.jl.

[tylerjthomas9]

So, can you confirm that this is a problem with Julia 1.8.4 generally? If so, I will merge this since the failure almost certainly has absolutely nothing to do with XGBoost.jl.

Yes, this is not an XGBoost.jl issue. Something has to be changed on the julia/XGBoost_jll side. LIBSVM.jl is having the same exact issue.

I do not think a recipe upgrade for XGBoost_jll will fix it, but that is a Yggdrasil question, not XGBoost.jl. Most likely, a change has to be made with julia.

[ExpandingMan]

Good enough for me.