add kwarg constructor - Githubissues

svilupp commented 1 year ago

Fixes https://github.com/Evovest/EvoTrees.jl/issues/216

This PR should be ready for review.

Until the issue with parametric kwargs is fixed upstream in MLJTreeParzen, these constructors will prevent hard-to-debug errors for new users.

Changes:

added kwarg constructor for parametric types, eg, EvoTreeRegressor{Linear,Float32}(; kwargs...)
added related tests
I did a manual formatting clean-up to match the style of the rest of the code base (JuliaFormatter.toml is missing)

Tests:

Core tests pass
MLJ integration tests fail on an unrelated issue -- Should I open a separate issue? I think we're missing some compat bounds (more below)

Test failure (unrelated to the PR):

I get test failure outside of @test, UndefVarError: make_binary not defined (same for make_count)
It's caused by my test environment using MLJTestIntegration v0.1.0 but these functions were introduced only later
I believe a compat entry should be added for at least 0.2.0 or above

svilupp commented 1 year ago

Looking at the failing tests, it all seems to be related to the missing compat for MLJTestIntegration mentioned above. I've added a compat for 0.4 to see if it helps.

jeremiedb commented 1 year ago

I wouldn't want to add MLJTestIntegration as a dependency to the package per se, as I'm loooking to keep the dependencies as light as possible.

An approach could be to go for a Project.toml in the test/ directory, so as to add a specific compat, instead of the extras approach used so far.

However, I quite don't understand why MLJTestIntegration resolves to v0.1 while it has not been an issue up to now. Tests also work fine when I run them locally, so I'm a little worried there's something unexpected that has changed. Any idea on that?

svilupp commented 1 year ago

However, I quite don't understand why MLJTestIntegration resolves to v0.1 while it has not been an issue up to now.

I’ll look into it.

I assumed it was just cached somewhere, but I’ll check the dep graphs. It’s unlikely to be caused by this PR, as I haven't touched any of the failing bits or imports, so maybe something upstream?

I wouldn't want to add MLJTestIntegration as a dependency to the package per se, as I'm loooking to keep the dependencies as light as possible.

Could you please clarify this? I didn’t change the deps organization (it was already in extras for testing the MLJ integration), so nothing was added?

An approach could be to go for a Project.toml in the test/ directory, so as to add a specific compat, instead of the extras approach used so far.

I thought this is equivalent. Is it not? Ie, it’s installed only when testing, but the difference is that if you have dedicated Test Env, you’ll have to update your compats in two places

svilupp commented 1 year ago

So I've used TestEnv to explore the test environment on main branch. The same issue happens -> tests fail because of outdated MLJTestIntegration (0.1.0)

This is the status of ]st -m --outdated

⌅ [30fc2ffe] LossFunctions v0.8.1 (<v0.9.0): MLJBase
⌅ [add582a8] MLJ v0.18.6 (<v0.19.1): MLJTestIntegration
⌅ [a7f614a8] MLJBase v0.20.20 (<v0.21.9): MLJ
⌅ [d491faf4] MLJModels v0.15.15 (<v0.16.6): MLJ
⌃ [697918b4] MLJTestIntegration v0.1.0 (<v0.4.0)
⌅ [458c3c95] OpenSSL_jll v1.1.20+0 (<v3.0.8+0): OpenSSL

There is nothing blocking the update (and the other MLJ deps are held back by MLJTestIntegration).

After running the update, all tests pass.

If I delete my Manifest.toml and instantiate again, the error appears again (because the environment automatically picks 0.1.0 -- can't tell why).

svilupp commented 1 year ago

An approach could be to go for a Project.toml in the test/ directory, so as to add a specific compat, instead of the extras approach used so far.

I've extracted the test environment to a dedicated test/Project.toml as requested + updated the name of this PR to reflect the change in scope.

It fixes the testing failures on my machine. Let's see what the CI thinks.

Given the cleaning up nature, would you consider adding a JuliaFormatter.toml spec? (I'd do it in a separate PR not to mix too many things)

jeremiedb commented 1 year ago

Thanks for looking further into this. My understanding is that there must have been some recent changes in the MLJ's dependency stack that result in those conflicts. I think I'd prefer seeing this resolve on that end rather change the Test dependency structure.

@ablaom The versioning conflict experience in this PR seems rooted from MLJTestIntegration.jl having a dependency on NearestNeighborModels which itself has a compat bound that doesn't include FillArrays recent v1 release. A patch release to cover v1 on NearestNeighborModels would fix this, though it I'm not sure whether it's intended to have NearestNeighborModels as a dependency in MLJTestIntegration as I expect such package not to depend on specific model implementation. Let me know what you think!

ablaom commented 1 year ago

You should not be using MLJTestIntegration.jl, as is loads the whole MLJ shebang.

Use MLJTestInterface.jl instead. See, for example, here: https://github.com/JuliaAI/MLJXGBoostInterface.jl/blob/4456438d134c20207c0c8383afb14d9f7bfa816c/test/runtests.jl#L205

svilupp commented 1 year ago

I've replaced MLJTestIntegration with MLJTestIntegration, all tests pass locally.

@jeremiedb,

I've kept the separate test environment, as per your request on minimizing the dependencies for EvoTrees
I've added a compat for MLJTestInterface to avoid unrelated test failures in the future (like what we had here)

jeremiedb commented 1 year ago

@svilupp Sorry for this apparent back and forth, but given the issue was from usage of MLJTestIntegration instead of MLJTestInterface, I'd rather move back to the original approach that use [extras] and [targets] in the main Project.toml. This would keep the Project in line with XGBoost implementation and the approach still used by most packages.

@ablaom Thanks for the clarification!

svilupp commented 1 year ago

@jeremiedb No problem. I've moved it back and all tests pass.

PR should be ready for review.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 93.75% and project coverage change: +0.47 :tada:

Comparison is base (f95e1b6) 47.45% compared to head (90fa6de) 47.92%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #221 +/- ## ========================================== + Coverage 47.45% 47.92% +0.47% ========================================== Files 20 20 Lines 1551 1567 +16 ========================================== + Hits 736 751 +15 - Misses 815 816 +1 ``` | [Impacted Files](https://codecov.io/gh/Evovest/EvoTrees.jl/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/models.jl](https://codecov.io/gh/Evovest/EvoTrees.jl/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL21vZGVscy5qbA==) | `96.29% <93.75%> (-0.45%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jeremiedb commented 1 year ago

@svilupp Looks good to go, thanks!

Evovest / EvoTrees.jl

add kwarg constructor #221

Codecov Report