ESMValGroup / ESMValTool

ESMValTool: A community diagnostic and performance metrics tool for routine evaluation of Earth system models in CMIP
https://www.esmvaltool.org
Apache License 2.0
210 stars 122 forks source link

ESMValTool v2.10.0 environment on JASMIN causes test failures #3588

Closed ehogan closed 1 month ago

ehogan commented 2 months ago

Describe the bug @mo-gill is currently working on installing ESMValTool v2.10.0 at the Met Office.

However, our first attempt to create an ESMValTool v2.10.0 environment caused some of the tests fail. So I thought I'd check the ESMValTool v2.10.0 environment on JASMIN. There are test failures on JASMIN too:

[...]
---------- Generated html report: file:///home/users/ehogan/software/ESMValTool/test-reports/report.html -----------
============================================= short test summary info ==============================================
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline0-kwargs0-None] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedRFE::test_advanced_rfe_no_fit_kwargs - AssertionError: assert [('passthroug...o-one'), [0])] == [('passthroug...hrough', [0])]
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedRFE::test_advanced_rfe_fit_kwargs - AssertionError: assert [('passthroug...o-one'), [0])] == [('passthroug...hrough', [0])]
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline1-kwargs1-output1] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline2-kwargs2-output2] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline3-kwargs3-output3] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline6-kwargs6-output6] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/test_diagnostic_run.py::test_diagnostic_run[diagnostic.jl] - SystemExit: 1
======================= 9 failed, 1453 passed, 1 skipped, 584 warnings in 241.91s (0:04:01) ========================

The MO failures are due to issues that have since been fixed in ESMValTool:

It looks like the first of these is causing the test failures on JASMIN.

I don't know how environments are created on JASMIN, but we fixed this at the MO by including a scikit-learn=1.3.2 pin (which is the version used in the lock file in v2.10.0) and adding <8.0 to the pytest specification.

Should the JASMIN environment be updated such that the tests pass? πŸ€”

valeriupredoi commented 1 month ago

hi @ehogan you are absolutely correct: on JASMIN we got:

scikit-learn              1.4.0           py311hc009520_0    conda-forge

which is no bueno; but given that nobody's yet complained about this issue, and it's a bit of a faff for me to update the environment, given 2.11 is knocking on the door and I'll have to install it anyway, I'll do the update when I get to install 2.11. Cheers for pointing out :beer:

valeriupredoi commented 1 month ago

for 2.10 you can use the lock file that comes shipped with it - safest bet that way (even if the lock file itself may predate the release by a few days since it gets built every week or so) :+1:

valeriupredoi commented 1 month ago

also, I just checked my install log for 2.10 centrally on JASMIN and lo and behold I have this:

Test fails list
===============
skikit-learn related test fails (normal)

---------------- Generated html report: file:///apps/jasmin/community/esmvaltool/ESMValTool_2.10.0/test-reports/report.html -----------------
========================================================== short test summary info ==========================================================
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline0-kwargs0-None] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline1-kwargs1-output1] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline2-kwargs2-output2] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline3-kwargs3-output3] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit_transformers_only[pipeline6-kwargs6-output6] - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedRFE::test_advanced_rfe_no_fit_kwargs - AssertionError: assert [('passthroug...o-one'), [0])] == [('passthroug...hrough', [0])]
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedRFE::test_advanced_rfe_fit_kwargs - AssertionError: assert [('passthroug...o-one'), [0])] == [('passthroug...hrough', [0])]
FAILED tests/integration/diag_scripts/mlr/test_custom_sklearn_classes.py::TestAdvancedPipeline::test_fit - TypeError: Pipeline._fit() got an unexpected keyword argument 't'
==================================== 8 failed, 1454 passed, 1 skipped, 78 warnings in 577.01s (0:09:37) ===

so it appears we considered those fails normal - I now remember I had a chat with @schlunma and he'd mentioned those fails don't degrade the recipe runs (llikewise when I ran the tests just now I had 8, not 9 fails) so if you don't mind, am gonna close this and leave the environment as is, @ehogan :beer:

valeriupredoi commented 1 month ago

about Julia - Julia test diagnostic's been broken since ages, see this https://github.com/ESMValGroup/ESMValTool/pull/3476 - I've been trying to fix it but unfortunately I don't speak her language all too well :grin:

ehogan commented 1 month ago

[...] Cheers for pointing out 🍺

[...] so if you don't mind, am gonna close this and leave the environment as is, @ehogan 🍺

No problem at all! Just thought I would do my due diligence and report it πŸ‘ 🍻

about Julia - Julia test diagnostic's been broken since ages, see this https://github.com/ESMValGroup/ESMValTool/pull/3476 - I've been trying to fix it but unfortunately I don't speak her language all too well 😁

Our failure is because we don't install Julia at the MO; I was actually thinking about opening an issue to skip the Julia test if it's not installed. Would that be reasonable? πŸ€”

schlunma commented 1 month ago

Yeah, to make them pass in our v2.10.0, we would have needed to pin sklearn<1.4.0, which we didn't do (because we were not aware of that issue at the time). So if you do a fresh installation of v2.10 right now, you get sklearn>=1.4.0, which will cause the tests to fail.

The proper way to fix this is probably a repodata patch (e.g. https://github.com/conda-forge/conda-forge-repodata-patches-feedstock/pull/455). This has been properly solved in v2.11 in our code.

valeriupredoi commented 1 month ago

[...] Cheers for pointing out 🍺

[...] so if you don't mind, am gonna close this and leave the environment as is, @ehogan 🍺

No problem at all! Just thought I would do my due diligence and report it πŸ‘ 🍻

excellent! We need more of these :smiley:

about Julia - Julia test diagnostic's been broken since ages, see this #3476 - I've been trying to fix it but unfortunately I don't speak her language all too well 😁

Our failure is because we don't install Julia at the MO; I was actually thinking about opening an issue to skip the Julia test if it's not installed. Would that be reasonable? πŸ€”

why not? It's a PITA and we only have like one recipe that nobody runs :rofl:

valeriupredoi commented 1 month ago

Manu, I can't be arsed to do a repodata patch, unless you tell me it's vital :grin:

ehogan commented 1 month ago

about Julia - Julia test diagnostic's been broken since ages, see this #3476 - I've been trying to fix it but unfortunately I don't speak her language all too well 😁

Our failure is because we don't install Julia at the MO; I was actually thinking about opening an issue to skip the Julia test if it's not installed. Would that be reasonable? πŸ€”

why not? It's a PITA and we only have like one recipe that nobody runs 🀣

New issue opened: https://github.com/ESMValGroup/ESMValTool/issues/3599 πŸ₯³

schlunma commented 1 month ago

Manu, I can't be arsed to do a repodata patch, unless you tell me it's vital 😁

No, it's certainly not 😁

valeriupredoi commented 1 month ago

@schlunma aintRDP