lots of small fixes for cli interaction #1330
Conversation
|
This is an important PR that we should merge for a 0.12.1 release to fix broken cmdline interaction (e.g. visualisation script) |
docs/policies.rst
Outdated
| --------------------- | ||
|
|
||
| By default, we try to provide you with a good set of configuration values | ||
| and policies that suite most people. But you are encouraged to modify |
rasa_core/agent.py
Outdated
|
|
||
| return max(max_histories, default=0) | ||
|
|
||
| def _are_all_featurizes_using_a_max_history(self): |
| max_max_history) | ||
| elif policy.featurizer is not None: | ||
| all_max_history_featurizers = False | ||
| max_history = self._max_history() |
There was a problem hiding this comment.
this is a lot more readable now!
| from __future__ import absolute_import | ||
| from __future__ import division | ||
| from __future__ import print_function | ||
| from __future__ import unicode_literals |
rasa_core/train.py
Outdated
| if not cmdline_args.out: | ||
| raise ValueError("you must provide a path where the model " | ||
| "will be saved using -o / --out") | ||
|
|
||
| if (isinstance(cmdline_args.config, list) and | ||
| len(cmdline_args.config) > 1): | ||
| raise ValueError("You can only pass one config file at a time") |
There was a problem hiding this comment.
We could do away with these checks by leaving nargs for config unspecified, in which case it is passed as a single string, not a list. The script won't even run if you'd specify more
There was a problem hiding this comment.
yes, that is a good point, at the moment we really do not need multiple configs for this script
rasa_core/cli/arguments.py
Outdated
| parser.add_argument( | ||
| '-c', '--config', | ||
| type=str, | ||
| nargs="*", |
There was a problem hiding this comment.
Can nargs be anything other than one (or the default)?
rasa_core/cli/arguments.py
Outdated
| type=str, | ||
| required=True, | ||
| help="domain specification yaml file") | ||
| return parser |
There was a problem hiding this comment.
Can we make these functions return None to indicate that parser is mutable? We don't use the return value in any of these functions' calls
rasa_core/config.py
Outdated
| @@ -11,10 +11,8 @@ | |||
|
|
|||
| def load(config_file): | |||
| # type: (Optional[Text], Dict[Text, Any], int) -> List[Policy] | |||
There was a problem hiding this comment.
the type annotations don't seem right
rasa_core/evaluate.py
Outdated
| @@ -605,7 +599,7 @@ def run_comparison_evaluation(models, stories, output): | |||
| num_correct) | |||
|
|
|||
|
|
|||
| def plot_curve(output, no_stories, ax=None, **kwargs): | |||
| def plot_curve(output, no_stories, ax=None): | |||
| """Plot the results from run_comparison_evaluation.""" | |||
There was a problem hiding this comment.
Can we add type annotations and include the arguments in the description?
|
Alright, done - ready for another review. I took your comment (adding description to parameters) as a reason to bring all the comments in the same google style docstr format. |
| :param app_password: Bot Framework application secret | ||
| Args: | ||
| app_id: Bot Framework's API id | ||
| app_password: Bot Framework application secret |
Proposed changes:
Status (please check what you already did):