rasa train --dry-run #7397
rasa train --dry-run #7397
Conversation
|
Going to add one more test to it, but in general it's done and ready to be reviewed. |
rasa/train.py
Outdated
| code, texts = dry_run_result(fingerprint_comparison) | ||
| for text in texts: | ||
| print_warning(text) if code > 0 else print_success(text) | ||
| sys.exit(code) |
There was a problem hiding this comment.
This might be a real PITA, but I wonder if the code should be passed up to rasa/cli/train.py and exiting handled there? Just as rasa/cli/ where we tend to handle things like this.
WDYT?
There was a problem hiding this comment.
It's definitely not a PITA, and might actually be a good idea :)
However, not sure if I have time to check it today — and in this case I'd be able to update it only in a week.
tests/cli/test_rasa_train.py
Outdated
| assert not any([s for s in printed_output if "No training required." in s]) | ||
| assert ( | ||
| ( | ||
| output.ret & CODE_CORE_NEEDS_TO_BE_RETRAINED |
There was a problem hiding this comment.
this is pretty cool. Is this a conventional way of handling shell exit codes?
There was a problem hiding this comment.
Well, afaik the standard exit code is 0 for success and any number from 1 to 255 for anything else.
I thought that it might be a good idea to just use bit masks to encode all the information we need because we have 4 booleans in FingerprintComparisonResult (which gives us 2^4 possible results).
rasa/cli/arguments/train.py
Outdated
| action="store_true", | ||
| help="If enabled, not actual training will be performed. Instead, " | ||
| "it will be determined whether a model should be re-trained " | ||
| "and this information will be printed as an output.", |
There was a problem hiding this comment.
Should we explain the different exit codes?
rasa/cli/arguments/train.py
Outdated
| "and this information will be printed as the output. The return " | ||
| "code is a 4-bit bitmask that can also be used to determine what exactly needs " | ||
| "to be retrained:\n" | ||
| "- 0b0001 means Core needs to be retrained\n" |
There was a problem hiding this comment.
Let's just tell them the integer which the bitmask translates to. No need to refresh their knowledge on binary numbers 😂
| @@ -0,0 +1,2 @@ | |||
| Add `rasa train --dry-run` command that allows to check if training needs to be performed | |||
There was a problem hiding this comment.
For consistency it might be an idea to also add this to rasa train core & rasa train nlu, what do you think?
There was a problem hiding this comment.
Well, we can do that but I think it makes very little sense.
In addition, the results of rasa train core --dry-run will be different and will basically be just answering a question of "does the core model need to be retrained?" in this case.
But I don't have a strong opinion on that — on one hand, I understand your argument about the consistency (however, in this case we might also consider adding --force argument to rasa train nlu) but on the other hand I don't understand what problem does it solve and how it can be useful :)
There was a problem hiding this comment.
Yeah, I was wondering the same 👍 Let's not cover them then.
| @@ -0,0 +1,2 @@ | |||
| Add `rasa train --dry-run` command that allows to check if training needs to be performed | |||
There was a problem hiding this comment.
Yeah, I was wondering the same 👍 Let's not cover them then.
|
Could not update branch. Most likely this is due to a merge conflict. Please update the branch manually and fix any issues. |
� Conflicts: � rasa/server.py � rasa/train.py � tests/conftest.py � tests/test_server.py
Proposed changes:
rasa train --dry-runcommand (part of Run training fingerprinting via the CLI #6729)Status (please check what you already did):
black(please check Readme for instructions)