Fix failing mlr diagnostic test

[remi-kazeroni]

Description

Adding 'array_api_support': False, to the list of DEFAULT_TAGS in the failing test test_safe_tags_no_get_tags seems to fix the issue locally and allows to use the latest version (scikit-learn 1.3.0).

• Closes Failing mlr diagnostic script test #3269

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

• 🛠 This pull request has a descriptive title
• 🛠 Code is written according to the code quality guidelines
• 🛠 Tests run successfully
• 🛠 All checks below this pull request were successful

---

To help with the number of pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[valeriupredoi]

so indeed this is an aftereffect of the new scikit-learn=1.3.0 (just tested for this myself) - cheers for the fix, Remi, it looks fine to me - we may have to tell @schlunma the new version 1.3.0 seems to be a bit buggy (have seen at least one breaking change, undocumented, rased as an issue there), also scipy moved to 1.11.1 that may affect scikit-learn

[valeriupredoi]

@bouweandela if you could please have a looksee and merge, this will have brought all our tests to green now that Conda install for py39 works again (by means of yanking scipy 1.11.0 and replacing it with 1.11.1 - yes they really yanked 1.11.0) 🟢

[bouweandela]

@remi-kazeroni Thanks for fixing the problem! Would it be possible to add a brief explanation of why this change is needed?

[valeriupredoi]

for @remi-kazeroni - I suspect the need to specify that kwarg to be a bug - was introduced in scikit-learn/scikit-learn#26372 AFAICS but that should be picked up as default - looking at the Changelog it looks like that PR has not made it through to the 1.3 release?

[remi-kazeroni]

Fully agreed with @valeriupredoi. I don't understand why this particular tag is not picked up as default. It is indeed in the released version, see here. I'm afraid I don't have a good explanation for that, all I could figure out is already in the description of that PR.

@bouweandela - if you like, we could open an issue to ask @schlunma to double check (and maybe improve) this fix once he's back to work. If there is a bug on the dependency side, we could then report upstream.

[valeriupredoi]

Fully agreed with @valeriupredoi. I don't understand why this particular tag is not picked up as default. It is indeed in the released version, see here. I'm afraid I don't have a good explanation for that, all I could figure out is already in the description of that PR.

@bouweandela - if you like, we could open an issue to ask @schlunma to double check (and maybe improve) this fix once he's back to work. If there is a bug on the dependency side, we could then report upstream.

am with Remi 👍

[bouweandela]

I don't understand why this particular tag is not picked up as default. It is indeed in the released version, see here.

It looks like our code contains a copy of _DEFAULT_TAGS from the file you linked, it is located here:

ESMValTool/esmvaltool/diag_scripts/mlr/custom_sklearn.py

Lines 88 to 107 in 776135a

	_DEFAULT_TAGS = {
	'non_deterministic': False,
	'requires_positive_X': False,
	'requires_positive_y': False,
	'X_types': ['2darray'],
	'poor_score': False,
	'no_validation': False,
	'multioutput': False,
	"allow_nan": False,
	'stateless': False,
	'multilabel': False,
	'_skip_test': False,
	'_xfail_checks': False,
	'multioutput_only': False,
	'binary_only': False,
	'requires_fit': True,
	'preserves_dtype': [np.float64],
	'requires_y': False,
	'pairwise': False,
	}

I could also get the tests to pass by updating our copy of _DEFAULT_TAGS, but to be honest I don't understand what this does and have no time to dive into the details. Since you don't seem to understand either, shall we just mark the test as xfail and wait for the author to come back? Or do we expect recipes to fail with the latest version of scikit-learn?

[valeriupredoi]

if we are going the safe route, and stop being dashing and glamorous, then I would rather pin scikit-learn <1.3.0 - it looks like that new version contains a few landmines for others too 👍

[remi-kazeroni]

if we are going the safe route, and stop being dashing and glamorous, then I would rather pin scikit-learn <1.3.0 - it looks like that new version contains a few landmines for others too 👍

Agreed. Since we don't have @schlunma with us this week to check whether his recipes are affected by this latest version of scikit-learn, I would recommend to pin unless you think that is problematic for other reasons. We could let @schlunma unpin once back to work.

[valeriupredoi]

I propose the following, pending approval from the RM (le @bouweandela ) and RM-junior (le @remi-kazeroni ):

• pin in the development environment
• don't pin in the conda recipe so that we are not stuck with a possibly unwanted pin and force a bugfix release to unpin

What say you guys?

[bouweandela]

After all trouble we had last month getting rid of some upper pins, I'm not keen to introduce new ones. I'm planning to run all recipes again anyway before the release, so let's not jump to conclusions about pinning here.

[valeriupredoi]

the " 🍍 -Agnostic RM" 🤣 Suits me fine, but we must either xfail the test or plop this workaround otherwise Lady Conda will be groaning and moaning

[remi-kazeroni]

see #3273 for a second tentative fix. This PR can be closed if #3273 leads to a consensus.

[remi-kazeroni]

Not needed