[ENH] in mlflow plugin, improve informativity of ModuleNotFoundError messages

[achieveordie]

Reference Issues/PRs

Fixes #5417

What does this implement/fix? Explain your changes.

I believe the original ModuleNotFoundError message was not accurate for mlflow since the error it gave made it seem like mlflow can be installed in the soft-dependencies bundle. I've added a more accurate message that points the user to use pip install sktime[mlflow].

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions

• I've added myself to the list of contributors with any new badges I've earned :-)
  How to: add yourself to the all-contributors file in the sktime root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
  See here for full badge reference
• Optionally, I've added myself and possibly others to the CODEOWNERS file - do this if you want to become the owner or maintainer of an estimator you added.
  See here for further details on the algorithm maintainer role.
• The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

For new estimators

• I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
• I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
• If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
  dependency isolation, see the estimator dependencies guide.

[fkiraly]

Thx, good idea!

Although to minimize coupling, I would not change the _check_soft_dependencies utility, but instead use the obj argument to override the error message in the downstream case.

[achieveordie]

Sorry, could you explain what you mean by "but instead use the obj argument to override the error message in the downstream case"? As far as I understand, we need to throw this error whenever a mlflow-related functionality is invoked. All these functionalities should have a _check_soft_dependencies("mlflow", severity="error") line which checks whether this package is present in the current environment or not.

Adding the message at the origin of the error seems like the most obvious way to go about it but I may have misunderstood your point.

[fkiraly]

Adding the message at the origin of the error seems like the most obvious way to go about it but I may have misunderstood your point.

Sorry, I meant using the msg, not the obj variable, I misspoke.
The msg argument is the one that allows you to provide a custom error message.

Personally, I don't think adding the message at the very origin of the error is the best way to go here, as it introduces coupling between the calling location and the called logic. As the called method already has an argument that allows you to specify the error message, we should imo simply use that.

[achieveordie]

As the called method already has an argument that allows you to specify the error message, we should imo simply use that.

Wouldn't that increase the amount of duplication for every _check_soft_dependencies("mlfllow") call in the codebase? This would also make updating this message more tedious because we no longer have a single point of origin. We already have 10 different points in the codebase where this line is being used.

An additional concern is where new developers are adding this line in their piece of code, they would also need to write the same error message or we'd get the original message again.

[fkiraly]

Wouldn't that increase the amount of duplication for every _check_soft_dependencies("mlfllow") call in the codebase?

Yes, you are right, it is not DRY.

Then, the solution which is both DRY and not coupled would be an _check_mlflow_dep which wraps _check_soft_dependencies with the desired message, similar to _check_dl_dependencies

[achieveordie]

Then, the solution which is both DRY and not coupled would be an _check_mlflow_dep which wraps _check_soft_dependencies with the desired message, similar to _check_dl_dependencies

Great idea!

[fkiraly]

Thanks!

Could you kindly add a full docstring for _check_mlflow_dependencies, as developers will likely encounter this at the top of their question stack when reading the mlflow plugin.

Non-blocking: in test skipif-s, I would still use _check_soft_dependencies, as it is easier to parse (name and similar to the same pattern elsewhere), and the error message does not matter due to severity="none".

[fkiraly]

I'd be happy with this now, thanks.