fix the "loading model" message which was logged twice #8278
Conversation
|
|
|
Thanks for submitting a pull request 🚀 @erohmensing will take a look at it as soon as possible ✨ |
|
Oops, this was the ignore list i was meant to add you to 🙈 |
…a run. change formatting fix model path return
rasa/model.py
Outdated
| def get_model(model_path: Text = DEFAULT_MODELS_PATH) -> TempDirectoryPath: | ||
| """Get a model and unpack it. Raises a `ModelNotFound` exception if | ||
| no model could be found at the provided path. | ||
| def verify_model_path(model_path: Text = DEFAULT_MODELS_PATH) -> Text: |
There was a problem hiding this comment.
I like your clean and precise change 💯
One thing:
I find the naming verify_model_path a little bit confusing as this function is doing two things: verifying the path and also returning the path. How about naming it get_local_model?
| """Get a model and unpack it. Raises a `ModelNotFound` exception if | ||
| no model could be found at the provided path. | ||
| def verify_model_path(model_path: Text = DEFAULT_MODELS_PATH) -> Text: | ||
| """Verifies that a model path exists. | ||
|
|
||
| Args: | ||
| model_path: Path to the zipped model. If it's a directory, the latest | ||
| trained model is returned. |
There was a problem hiding this comment.
please also document the Returns part (pydocstyle unfortunately has a bug which doesn't flag missing Returns 😬 )
tests/test_model.py
Outdated
| @@ -78,6 +79,12 @@ def test_get_model_context_manager(trained_rasa_model: str): | |||
| assert not os.path.exists(unpacked) | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("model_path", ["foobar", "rasa", "README.md", None]) | |||
| def test_verify_model_path_exception(model_path: Optional[Text]): | |||
There was a problem hiding this comment.
If you write this test then you probably also want one which checks with valid model files, don't you?
rasa/cli/run.py
Outdated
| @@ -94,7 +94,7 @@ def run(args: argparse.Namespace): | |||
| # make sure either a model server, a remote storage, or a local model is | |||
| # configured | |||
|
|
|||
| from rasa.model import get_model | |||
| from rasa.model import verify_model_path, get_model | |||
There was a problem hiding this comment.
small note: Our Python guide line states that we rather import the modules instead of the functions directly. This makes it a little bit easier to see where a function is coming from when reading the code.
rasa/model.py
Outdated
|
|
||
|
|
||
| def get_model(model_path: Text = DEFAULT_MODELS_PATH) -> TempDirectoryPath: | ||
| """Get a model and unpack it. |
There was a problem hiding this comment.
| """Get a model and unpack it. | |
| """Gets a model and unpack it. |
rasa/model.py
Outdated
|
|
||
| Args: | ||
| model_path: Path to the zipped model. If it's a directory, the latest | ||
| trained model is returned. | ||
|
|
||
| Returns: | ||
| Path to the unpacked model. | ||
| model_path: Path to the zipped model. If it's a directory, the latest |
There was a problem hiding this comment.
| model_path: Path to the zipped model. If it's a directory, the latest | |
| Path to the zipped model. If it's a directory, the latest |
rasa/model.py
Outdated
| """Get a model and unpack it. Raises a `ModelNotFound` exception if | ||
| no model could be found at the provided path. | ||
| def get_local_model(model_path: Text = DEFAULT_MODELS_PATH) -> Text: | ||
| """Verifies that a model path exists. |
There was a problem hiding this comment.
I think we need to adapt that one, too. How about:
| """Verifies that a model path exists. | |
| """Returns verified path to local model archive. |
tests/test_model.py
Outdated
| @@ -42,6 +42,7 @@ | |||
| can_finetune, | |||
| create_package_rasa, | |||
| get_latest_model, | |||
| get_local_model, | |||
There was a problem hiding this comment.
minor thing: Could we please also import the things by their module here instead of importing the functions directly?
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)