🐛 verdi code create: fix crash rendering help#7381
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7381 +/- ##
===========================================
- Coverage 80.38% 30.00% -50.38%
===========================================
Files 578 578
Lines 46132 46138 +6
===========================================
- Hits 37079 13839 -23240
- Misses 9053 32299 +23246 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`verdi code create` is a `DynamicEntryPointCommandGroup` that builds its subcommands from the `aiida.data` entry points matching `core.code.*`. This includes `core.code.abstract`, which resolves to the abstract base class `AbstractCode`. Since `AbstractCode` cannot be instantiated, its `CliModel` property raises `UnsupportedSchemaError`, so building the command options for it crashed the rendering of the whole group, e.g. `verdi code create -h` or `--help`. Exclude entry points whose class is abstract (in addition to the existing `cli_exposed` opt-out) when listing and resolving subcommands. Abstract plugins are now omitted from the help, and resolving one directly fails with the regular user-facing "is not a command" error with suggestions, instead of an internal traceback. Fixes aiidateam#7379
aad9ca1 to
e7a07da
Compare
|
Thanks, @officialasishkumar. I just updated the branch (I didn't really get why CI was failing, and updating and re-triggering often can fix it), and as I did a rebase, I'm now shown as co-author. Sorry about that, I'll remove myself again when merging this, as this is your contribution =) |
| assert 'core.code.installed' in result.output | ||
| assert 'core.code.portable' in result.output | ||
| assert 'core.code.abstract' not in result.output |
There was a problem hiding this comment.
Nit: Could also assert for containerized code here
| assert 'core.code.installed' in result.output | |
| assert 'core.code.portable' in result.output | |
| assert 'core.code.abstract' not in result.output | |
| assert 'core.code.containerized' in result.output | |
| assert 'core.code.installed' in result.output | |
| assert 'core.code.portable' in result.output | |
| assert 'core.code.abstract' not in result.output |
There was a problem hiding this comment.
Non-blocking nit on the two new tests here: test_list_commands_excludes_non_exposed and test_get_command_non_exposed share the same setup and a near-identical docstring, and they're really two facets of one contract ("non-exposed plugins are neither listed nor resolvable"). They could collapse into a fixture + one parametrized test across the subjects:
fixture + parametrized test_subcommand_exposure
@pytest.fixture
def custom_group(entry_points):
"""A dynamic group with one concrete, one abstract, and one ``cli_exposed = False`` plugin registered."""
entry_points.add(CustomClass, 'aiida.custom:custom')
entry_points.add(AbstractCustomClass, 'aiida.custom:abstract')
entry_points.add(HiddenCustomClass, 'aiida.custom:hidden')
return DynamicEntryPointCommandGroup(lambda *args, **kwargs: True, name='create', entry_point_group='aiida.custom')
@pytest.mark.parametrize(
'cmd_name, exposed',
[
pytest.param('custom', True, id='concrete'),
pytest.param('abstract', False, id='abstract'),
pytest.param('hidden', False, id='cli_exposed_false'),
pytest.param('non_existent', False, id='unknown'),
],
)
def test_subcommand_exposure(custom_group, cmd_name, exposed):
"""Only exposed plugins are listed and resolvable; abstract / ``cli_exposed = False`` / unknown are excluded.
Regression test for https://github.com/aiidateam/aiida-core/issues/7379: an abstract entry point cannot be
instantiated, so building its CLI options raised ``UnsupportedSchemaError`` and broke rendering of the whole
group. Such plugins must be silently dropped from the listing and resolve to the regular user-facing
:class:`click.exceptions.UsageError` instead of an internal traceback.
"""
ctx = click.Context(custom_group)
assert (cmd_name in custom_group.list_commands(ctx)) is exposed
if exposed:
assert custom_group.get_command(ctx, cmd_name) is not None
else:
with pytest.raises(click.exceptions.UsageError):
custom_group.get_command(ctx, cmd_name)No loss of coverage: since only those three are registered and the group has no static commands, asserting custom in / abstract not in / hidden not in pins list_commands == ['custom'] exactly, and it also folds in the positive get_command('custom') case and the non_existent path. That said, this is just my personal preference and it's your call to take it or leave it =)
|
Hm, was a bit too trigger happy here now. I see CI is failing. Ofc, we'll need to fix that before we can merge. Currently looking into it, out of curiosity. It's not immediately evident to me, from the code changes here, why it should start crashing the tests... maybe one needs to adapt some other test, elsewhere. EDIT: found it, and I don't think it's a bug in this PR, it's surfacing a now-wrong test assertion. The 4 Looping in @edan-bainglass: should |
|
Just posting this here, as I still had it pending locally. Thanks a lot for the PR, @officialasishkumar! Once CI passes, I'll approve again. I think this is the ideal solution (for now, up until v3, in which we redesign the
|
Fixes #7379
Problem
Running
verdi code create -h(or--help) crashes with anUnsupportedSchemaErrorinstead of showing the help menu:Root cause
verdi code createis aDynamicEntryPointCommandGroupthat builds its subcommands from theaiida.dataentry points matchingcore.code.*. That filter also matchescore.code.abstract, which resolves to the abstract base classAbstractCode.When
clickrenders the group help, it callsget_commandfor every listed subcommand in order to extract its short help, which builds the command's options vialist_options. ForAbstractCodethis accesses theCliModelproperty, which deliberately raisesUnsupportedSchemaErrorbecause an abstract code cannot be instantiated.list_optionsreads it withgetattr(cls, 'CliModel', None), butgetattronly suppressesAttributeError, so the exception propagates and breaks the rendering of the whole group.Fix
Exclude entry points whose class is abstract when listing and resolving the subcommands of a
DynamicEntryPointCommandGroup. This is done withinspect.isabstract, in addition to the existingcli_exposedopt-out (used e.g. by thecore.sqlite_tempstorage backend). The two checks are factored into a small_is_exposedhelper that is shared bylist_commandsandget_command.As a result:
verdi code create -hnow renders the help and lists the concrete plugins (core.code.installed,core.code.portable,core.code.containerized).verdi code create core.code.abstract) now fails with the regular user-facingis not a create commanderror (with suggestions) instead of an internal traceback.The fix is generic and also protects the other dynamic group (
verdi profile setup) and any third-party plugin that registers an abstract base class under a CLI entry point group.Tests
tests/cmdline/groups/test_dynamic.py: unit tests asserting that abstract andcli_exposed = Falseplugin classes are excluded fromlist_commandsand that resolving them raises the user-facingUsageErrorrather than the internalUnsupportedSchemaError.tests/cmdline/commands/test_code.py::test_code_create_help: regression test thatverdi code create --helprenders and lists the concrete code plugins but notcore.code.abstract.All of
tests/cmdline/commands/test_code.py,tests/cmdline/groups/test_dynamic.pyandtests/cmdline/commands/test_presto.pypass locally, andpre-commit(ruff, mypy) is clean.