[BUG] check_dependency for soft dependencies if defined with version in tags is failing

[benHeid]

If you add a new estimator with a soft dependency specifying a certain version then it will fail, since it is not checked against a certain version, instead it is checked against an empty string: See: https://github.com/sktime/sktime/blob/63a95f2c29251575b7963ad269044654cbe9d96c/sktime/utils/validation/_dependencies.py#L177C1-L178C1

Furthermore if the check fails, the msg creation may raise an exception, since the module is not fully initialized when trying to get the string representation of the estimator.

See: #5887

[fkiraly]

Hm, I don't fully get it - what is actual behaviour and what is expected behaviour?

[benHeid]

In the PR for the adapter for tsbootstrap. I defined a lower bound for the tsbootstrap package. However, this check is not working (for testing I used tsbootstrap>=0.0.3. The error message was: AttributeError: 'TSBootstrapAdapter' object has no attribute 'bootstrapper'

Thus, basically there are two bugs:

1. The error message should not be: AttributeError: 'TSBootstrapAdapter' object has no attribute 'bootstrapper'. Instead it should be something about that the versions do not match. The reason for this bug is that the check is triggered because in the __init__ first the __init__ of the parent class is called before the parameters are set (as recommended for python). The init call is then calling the check function. Since this happens before all parameters are set, the creation of the error message that the dependency versions are not fulfilled is failing.
2. Main Bug: The check is failing even if the a suitable version is installed. Since the linked check is wrong. If I understand it correctly, it checks the required version against a set defined by an empty string: https://github.com/sktime/sktime/blob/63a95f2c29251575b7963ad269044654cbe9d96c/sktime/utils/validation/_dependencies.py#L177C1-L178C1 To reproduce it,
   a. Install tsbootstrap using the fix from Add version dunder astrogilda/tsbootstrap#77
   b. execute:

        from sktime.transformations.bootstrap import TSBootstrapAdapter
        from sktime.datasets import load_airline
        from sktime.utils.plotting import plot_series  # doctest: +SKIP
        from sktime.datasets import load_airline
        from sktime.transformations.bootstrap import TSBootstrapAdapter
        from tsbootstrap import MovingBlockBootstrap, MovingBlockBootstrapConfig
        y = load_airline()
        bootstrap = TSBootstrapAdapter(MovingBlockBootstrap(MovingBlockBootstrapConfig(10, n_bootstraps=10)))
        result = bootstrap.fit_transform(y)

[fkiraly]

can you link the Ci failure?

might be that the test which fails does so due to inconsitency between init args and self params, e.g., bootstrap vs bootstrapper.

[benHeid]

can you link the Ci failure?

Unfortunately, not now.. Since it requires a specific branch of tsbootstrap. I try to specify this version in the pyproject.toml.

might be that the test which fails does so due to inconsitency between init args and self params, e.g., bootstrap vs bootstrapper.

No, the bug ist not associated with the initialisation. Either, I do not specify the version not correctly or there is a problem in the check_dependency function.

[fkiraly]

hm, could you post the traceback then? copy-paste?

[benHeid]

Stack trace

  File "/Users/benediktheidrich/code/sktime/scratches/from sktime.transformations.py", line 8, in <module>
    bootstrap = TSBootstrapAdapter(MovingBlockBootstrap(MovingBlockBootstrapConfig(10, n_bootstraps=10)))
  File "/Users/benediktheidrich/code/sktime/sktime/transformations/bootstrap/_tsbootstrap.py", line 55, in __init__
    super().__init__()
  File "/Users/benediktheidrich/code/sktime/sktime/transformations/base.py", line 209, in __init__
    _check_estimator_deps(self)
  File "/Users/benediktheidrich/code/sktime/sktime/utils/validation/_dependencies.py", line 433, in _check_estimator_deps
    pkg_deps_ok = _check_soft_dependencies(
  File "/Users/benediktheidrich/code/sktime/sktime/utils/validation/_dependencies.py", line 187, in _check_soft_dependencies
    f"This version requirement is not one by sktime, but specific "
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/site-packages/skbase/base/_base.py", line 902, in __repr__
    repr_ = pp.pformat(self)
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/pprint.py", line 157, in pformat
    self._format(object, sio, 0, 0, {}, 0)
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/pprint.py", line 174, in _format
    rep = self._repr(object, context, level)
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/pprint.py", line 454, in _repr
    repr, readable, recursive = self.format(object, context.copy(),
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/site-packages/skbase/base/_pretty_printing/_pprint.py", line 134, in format
    return _safe_repr(
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/site-packages/skbase/base/_pretty_printing/_pprint.py", line 389, in _safe_repr
    params = _changed_params(obj)
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/site-packages/skbase/base/_pretty_printing/_pprint.py", line 34, in _changed_params
    params = base_object.get_params(deep=False)
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/site-packages/skbase/base/_base.py", line 359, in get_params
    params = {key: getattr(self, key) for key in self.get_param_names()}
  File "/Users/benediktheidrich/anaconda3/envs/sktime/lib/python3.10/site-packages/skbase/base/_base.py", line 359, in <dictcomp>
    params = {key: getattr(self, key) for key in self.get_param_names()}
AttributeError: 'TSBootstrapAdapter' object has no attribute 'bootstrapper'

[fkiraly]

I see - I think this is "circular failure".

Namely, for the error message related to the failing version, it tries to print self, which tries to get_params, but the class must be failing the contract to write self.param = param in __init__ (this should happen before the super.__init__ call), so the print triggers the failure.

This should not be a problem for users once the class passes all skbase contract tests, but I suppose it is confusing for devs?

[benHeid]

I see - I think this is "circular failure".

Namely, for the error message related to the failing version, it tries to print self, which tries to get_params, but the class must be failing the contract to write self.param = param in __init__ (this should happen before the super.__init__ call), so the print triggers the failure.

In general, it would not be a good idea to first assign the parameters and call the super().__init__() afterwards, since this might override set attributes of the object. Thus, it is difficult to resolve this circularity. I would prefer just extracting the name of the class instead of getting it's string representation.

This should not be a problem for users once the class passes all skbase contract tests, but I suppose it is confusing for devs?

I do not understand this statement. I think this is not associated to failing tests of skbase. Thus, I also assume that this might affect users, if they have not installed a correct dependency of the third party library.

[fkiraly]

I would prefer just extracting the name of the class instead of getting it's string representation.

I agree partially, though that is not a perfect solution since the dependency tags may depend on parameters. Admittedly a rare case, but in this case the message would be wrong/misleading.

How about doing one of the following:

• in _check_soft_dependencies try/except, and if fails use the clsas name
• and/or do the same in __repr__

[benHeid]

How about doing one of the following:

• in _check_soft_dependencies try/except, and if fails use the clsas name
• and/or do the same in __repr__

Yes, doing this in __repr__ means that this should be done in skbase or? I think this is the better location for this change. That would fix the wrong error message.

Regarding the other part of the issue, which leads to exception even if the correct version is installed. Do you agree that the following line https://github.com/sktime/sktime/blob/63a95f2c29251575b7963ad269044654cbe9d96c/sktime/utils/validation/_dependencies.py#L177C1-L178C1 seems to be wrong?

[fkiraly]

Yes, doing this in __repr__ means that this should be done in skbase or? I think this is the better location for this change. That would fix the wrong error message.

Yes, in BaseObject of skbase.

Regarding the other part of the issue, which leads to exception even if the correct version is installed. Do you agree that the following line https://github.com/sktime/sktime/blob/63a95f2c29251575b7963ad269044654cbe9d96c/sktime/utils/validation/_dependencies.py#L177C1-L178C1 seems to be wrong?

Currently I cannot say, as I do not understand the rationale behind it anymore. I should have left a comment.

If I could hazard a guess, it is that the empty string results in the empty specifier which could be correct for the comparison?

[benHeid]

@fkiraly Here is the described error in the CI https://github.com/sktime/sktime/actions/runs/8030832840/job/21939354666

As mentioned, these are two bugs.

• Error message is not generated correctly
• Dependency check should not fail.

[fkiraly]

I'm confused - I think this is the interaction of __repr__ and writing params to self after the super.__init__ call, still?

At least, could you move the self.param = param to above the super.__init__ call, otherwise the error message is not specific to a failure of _check_estimator_deps imo.

[benHeid]

Yes, I can do this. However, I am not a fan of doing initialization before the parameters of the super class are initialized. But probably for the use-case of pretty printing this is okay.

[fkiraly]

However, I am not a fan of doing initialization before the parameters of the super class are initialized.

In the extension contract, it's this way round - perhaps it's the worse convention, but it's the one followed for years.

I think it has advantages in the sklearn / dataclass interface design, because that order allows one to assume in any non-boilerplate that self behaves like a dataclass.