-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix CLI and install instructions in case optional checklists is not present #5589
Conversation
""" | ||
The `checklist` subcommand allows you to conduct behavioural | ||
testing for your model's predictions using a trained model and its | ||
[`Predictor`](../predictors/predictor.md#predictor) wrapper. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a case where git does a better job than the github UI; in particular, this is a 100% copy of the file as it existed before ec96bef:
[cf ec96bef1] create dummy command if checklist not available
4 files changed, 51 insertions(+), 236 deletions(-)
copy allennlp/commands/{checklist.py => _checklist_internal.py} (100%)
rewrite allennlp/commands/checklist.py (94%)
rewrite allennlp/confidence_checks/task_checklists/__init__.py (100%)
Minus the commits for satisfying mypy and flake8, the state of this PR is what passed in conda-forge/allennlp-feedstock#35. I guess it would be good to add some CLI calls also here in the test suite, so that the functionality of the core-installation (i.e. without optional installs) is ensured by the CI here as well. Note that I'm not saying these patches are the only way to solve the problems - I'd be happy to iterate together on this... (IMO the fact that any CLI command ends up doing a full I do think that it would be a good idea to have a dummy command for
|
@AkshitaB, since this builds on your work, can you take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h-vetinari Thank you for working on this! I just have a minor suggestion.
@Subcommand.register("checklist") | ||
class CheckList(Subcommand): # type: ignore | ||
def add_subparser(self, parser: argparse._SubParsersAction) -> argparse.ArgumentParser: | ||
description = """Dummy command because checklist is not installed.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the installation info here, so that it shows up if someone does allennlp checklist --help
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @AkshitaB, thanks for taking a look.
That is indeed the intention, i.e. that any call to allennlp checklist
(when checklist
is missing) will print the installation instructions (see _dummy_output
above - name can be improved). I don't know the SubCommand
infrastructure well enough, but happy to adapt as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h-vetinari I think changing the description string to the _dummy_output
message should do the trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to print the installation tip unconditionally for any invocation of allennlp checklist
, not just for allennlp checklist --help
. Does that make sense? I'm happy to change it though if desired.
.github/workflows/ci.yml
Outdated
@@ -369,7 +369,7 @@ jobs: | |||
|
|||
- name: Install core package | |||
run: | | |||
pip install $(ls dist/*.whl)[all] | |||
pip install $(ls dist/*.whl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you want to have a full matrix of running against all installation flavours; IMO that would actually be a good idea...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is to run all tests including the checklist ones. So, we install [all] at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, though that's kind of the root of not catching #5588 through CI before, because currently something like allennlp test-install
is never tested without checklist
being present. Hence my recommendation that this should likely be expanded to a full matrix of runs against each allennlp-flavour (base, -checklist and any other flavours that might be added), to ensure the CLI keeps working for all of those variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that allennlp tests take a long time to run, I don't think we should run the full matrix for each flavor every time. @epwalsh Do you think it makes sense to do this when we make releases though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a compromise, how about we run the full matrix in CI for the test_package
job only?
allennlp/.github/workflows/ci.yml
Line 343 in e1c6935
test_package: |
That should catch issues like #5588
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did as suggested in e7d44a5. :)
Any open questions about this? I also think this would perhaps be reason enough for a v2.9.2 (not that we need it for conda-forge - we can carry the patch - but other people seem to have been bit by this as well)...? |
Thanks @h-vetinari, we'll do another release this week |
Following #5507
Fixes #5588, as tested in conda-forge/allennlp-feedstock#35.