Rasa: Clarify if component method required_packages returns module names or requirements

[merwok]

Rasa version: 1.0.2

Python version: 3.7.3

Operating system (windows, osx, ...): Debian testing

Issue: The doc at http://rasa.com/docs/rasa/api/custom-nlu-components/#rasa.nlu.components.Component.required_packages doesn’t make clear whether the strings returned should be module names (i.e. importable, like sklearn) or requirements (i.e. checked using packaging introspection methods, e.g. scikit-learn).

[akelad]

could you clarify what you mean? it specifies the names of python packages

[merwok]

«package» is ambiguous: is it something you install (requirement, e.g. scikit-learn, maybe with a version specifier) or something you import (Python module, e.g. sklearn)

[akelad]

We'd be happy to accept a PR phrasing this in a clearer way. This is the method that tries to import them fyi: https://github.com/RasaHQ/rasa/blob/master/rasa/nlu/components.py#L14

[merwok]

From the code, these are indeed importable packages, not dependencies in a packaging sense.

[akelad]

Yeah I know, are you willing to create a PR for this to make it clearer in the docs?

[merwok]

I found the source of the docs (https://github.com/RasaHQ/rasa/blob/master/rasa/nlu/components.py#L200-L204) but I think more work may be needed to avoid ambiguity here. For example, the method that checks these required modules may produce an error message that is super unclear about modules vs requirements: https://github.com/RasaHQ/rasa/blob/master/rasa/nlu/components.py#L28

[akelad]

ok before we look into this ourselves would you be willing to submit a PR for that as well? we're always happy about community contributions

[akelad]

ok before we start looking into this, would you be up for working on this as well? We're always happy to accept community contributions

[merwok]

Sure, but maybe not before a few weeks.

[hsm207]

I want to work on this

[hsm207]

I found the source of the docs (https://github.com/RasaHQ/rasa/blob/master/rasa/nlu/components.py#L200-L204) but I think more work may be needed to avoid ambiguity here. For example, the method that checks these required modules may produce an error message that is super unclear about modules vs requirements: https://github.com/RasaHQ/rasa/blob/master/rasa/nlu/components.py#L28

@merwok what are the changes that should be made? I don't see how the exception message raised by validate_requirements can be unclear.

[merwok]

I don’t know how to say the same thing in a different way :slightly_frowning_face:

To repeat my initial message: if import sklearn fails, it is not correct to say you need to run pip install sklearn, because the (importable) module name is not the same as the (installable) package name (here: scikit-learn).

[hsm207]

I don’t know how to say the same thing in a different way 🙁
To repeat my initial message: if import sklearn fails, it is not correct to say you need to run pip install sklearn, because the (importable) module name is not the same as the (installable) package name (here: scikit-learn).

But pip install sklearn will run and will actually install scikit-learn. In this case, it works because the package maintainer(s) have also uploaded a sklearn package to PyPi.

Do you have any other examples?

What do you think of updating the exception message in validate_requirements to something like:

Please install the package that contains the module sklearn

?

[merwok]

Maybe that particular example is not valid anymore :slightly_smiling_face: but the general idea still stands.

[hsm207]

So just update the exception message from validate_requirements is sufficient for now?

[merwok]

The exception message and also the doc I linked from my first message.

[hsm207]

I guess we can close this issue now since the pull request has been merged.

[merwok]

Thanks for improving the docs! :slightly_smiling_face: