feat: add --descendant and --ancestor flags to verdi node list - #7300
feat: add --descendant and --ancestor flags to verdi node list#7300Khushi281300 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Thanks for the contribution @Khushi281300, and sorry for the slow reply. I added a few comments. Please address them and then re-request review.
Please also make sure pre-commit passes locally. E.g., I just used the command below to run pre-commit on all your changes of this PR. You can find more info in our AGENTS.md and Claude skills. Thanks!
❯ uv run pre-commit run --from-ref $(git merge-base HEAD main) --to-ref HEAD
check for merge conflicts................................................Passed
check for added large files..............................................Passed
check yaml...........................................(no files to check)Skipped
fix double quoted strings................................................Passed
fix end of files.........................................................Passed
fix python encoding pragma (deprecated)..................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook
Fixing tests/cmdline/commands/test_node_list_relationship.py
Validate GitHub Workflows............................(no files to check)Skipped
ruff-format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook
1 file reformatted, 2 files left unchanged
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1
- files were modified by this hook
Fixed 2 errors:
- src/aiida/cmdline/params/options/main.py:
1 × RUF022 (unsorted-dunder-all)
- tests/cmdline/commands/test_node_list_relationship.py:
1 × I001 (unsorted-imports)
Found 2 errors (2 fixed, 0 remaining).
Pretty format TOML...................................(no files to check)Skipped
Pretty format YAML...................................(no files to check)Skipped
uv-lock..............................................(no files to check)Skipped
nbstripout...........................................(no files to check)Skipped
imports..................................................................Failed
- hook id: imports
- files were modified by this hook
mypy.....................................................................Passed
Update conda environment file........................(no files to check)Skipped
Validate environment.yml.............................(no files to check)Skipped
Automatically generating verdi docs......................................Passed
| ANCESTOR = OverridableOption( | ||
| '-a', '--ancestor', type=types.NodeParamType(), help='Filter for nodes that are an ancestor of this node.' | ||
| ) |
There was a problem hiding this comment.
The -a short flag is already used for --all, see here, and you're now reusing it for --ancestor. As these are both OverridableOptions that can be composed onto any command, the moment someone decorates a command with both @options.ALL() and @options.ANCESTOR(), click will blow up with a duplicate option error. Even if node_list doesn't currently use ALL, this is a landmine. I recommend dropping the short flag alltogether (see also my other comment).
| DESCENDANT = OverridableOption( | ||
| '-d', '--descendant', type=types.NodeParamType(), help='Filter for nodes that are a descendant of this node.' | ||
| ) | ||
|
|
||
|
|
||
| ANCESTOR = OverridableOption( | ||
| '-a', '--ancestor', type=types.NodeParamType(), help='Filter for nodes that are an ancestor of this node.' | ||
| ) |
There was a problem hiding this comment.
--descendant node_c actually returns node_c's ancestors (the query uses with_ancestors='node'). The current names read as "show me descendants of X", but the semantics are "show me nodes whose descendant is X". Renaming to --has-descendant / --has-ancestor makes it unambiguous: verdi node list --has-ancestor 42 clearly means "list nodes that have 42 as an ancestor". In addition, I'd fully drop the short flags.
For a demonstration, see this:
When using verdi node list --descendant 8, I actually get the ancestors of node 8, not the descendants (and vice versa). The current behavior is quite confusing.
| 'DESCENDANT', | ||
| 'ANCESTOR', |
There was a problem hiding this comment.
Also here, would become HAS_DESCENDANT and HAS_ANCESTOR. Also, please ensure the list is alphabetically sorted.
| @@ -0,0 +1,73 @@ | |||
| ########################################################################### | |||
There was a problem hiding this comment.
- Please don't create a new test file or fixture.
tests/cmdline/commands/test_node.pyalready hasTestNodeListforverdi node listtests. Thegenerate_calculation_nodefixture intests/conftest.pyalready creates a linked provenance graph (Data → Calc → Data) viainputs/outputskwargs. Use that instead of adding a newprovenance_graphfixture. Deletetests/cmdline/commands/test_node_list_relationship.pyand add a separate method onTestNodeListthat usesgenerate_calculation_node. Don't try to merge into the existing parametrizedtest_node_listsince that uses unlinked nodes with index-based expectations, which doesn't fit the relationship queries.
| node_a = orm.Data().store() | ||
| node_a.label = 'node_a' | ||
| node_a.store() |
There was a problem hiding this comment.
.store() called twice on the node here
| def node_list(entry_point, subclassing, project, past_days, order_by, order_dir, limit, raw): | ||
| @options.DESCENDANT() | ||
| @options.ANCESTOR() | ||
| def node_list(entry_point, subclassing, project, past_days, order_by, order_dir, limit, raw, descendant, ancestor): |
There was a problem hiding this comment.
Also rename here to has_descendant and has_ancestor in this file.
|
And, please also merge |
📝 WalkthroughWalkthroughChangesNode relationship filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as verdi node list
participant node_list
participant QueryBuilder
participant Node as Node provenance graph
CLI->>node_list: Pass relationship options
node_list->>QueryBuilder: Build filtered node query
QueryBuilder->>Node: Apply provenance constraints
Node-->>QueryBuilder: Return matching nodes
QueryBuilder-->>CLI: Return projected raw output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cmdline/commands/test_node_list_relationship.py (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required Sphinx-style docstrings.
Document fixture inputs and return values with
:param:/:return:directives, keeping any types in annotations rather than the docstrings. As per coding guidelines, “Use Sphinx-style docstrings (:param:,:return:,:raises:), with types written in annotations rather than docstrings.”Also applies to: 38-39, 51-52, 64-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cmdline/commands/test_node_list_relationship.py` around lines 16 - 20, Update the docstrings for the fixtures/functions around provenance_graph and the additionally affected sections to use Sphinx directives for every input and return value, adding :param: and :return: descriptions as applicable. Keep parameter and return types only in the Python annotations, not in the docstrings, and preserve the existing behavioral descriptions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cmdline/commands/test_node_list_relationship.py`:
- Line 40: Update the tuple unpacking of provenance_graph in each affected test
to assign unasserted node values to `_`, while retaining named variables for
nodes used in assertions. Apply this consistently at the unpacking statements
around lines 40, 53, and 66.
---
Nitpick comments:
In `@tests/cmdline/commands/test_node_list_relationship.py`:
- Around line 16-20: Update the docstrings for the fixtures/functions around
provenance_graph and the additionally affected sections to use Sphinx directives
for every input and return value, adding :param: and :return: descriptions as
applicable. Keep parameter and return types only in the Python annotations, not
in the docstrings, and preserve the existing behavioral descriptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a6f0252-dbf0-416a-a66e-603b0596e3f6
📒 Files selected for processing (3)
src/aiida/cmdline/commands/cmd_node.pysrc/aiida/cmdline/params/options/main.pytests/cmdline/commands/test_node_list_relationship.py
|
|
||
| def test_node_list_ancestor(run_cli_command, provenance_graph): | ||
| """Test the `--ancestor` filter.""" | ||
| node_a, node_b, node_c = provenance_graph |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the unused fixture values reported by Ruff.
Use _ for values not asserted by each test.
Proposed fix
- node_a, node_b, node_c = provenance_graph
+ node_a, _, _ = provenance_graph
...
- node_a, node_b, node_c = provenance_graph
+ _, _, node_c = provenance_graph
...
- node_a, node_b, node_c = provenance_graph
+ node_a, _, node_c = provenance_graphAlso applies to: 53-53, 66-66
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 40-40: Unpacked variable node_b is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 40-40: Unpacked variable node_c is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cmdline/commands/test_node_list_relationship.py` at line 40, Update the
tuple unpacking of provenance_graph in each affected test to assign unasserted
node values to `_`, while retaining named variables for nodes used in
assertions. Apply this consistently at the unpacking statements around lines 40,
53, and 66.
Source: Linters/SAST tools
2 participants
PR Description
Overview
This PR introduces the
--descendantand--ancestorflags toverdi node list, addressing issue #6782. These options allow users to explore the provenance graph directly from the CLI by filtering nodes based on their relationships.What’s Included
New CLI Options
Added
DESCENDANTandANCESTORoptions insrc/aiida/cmdline/params/options/main.pyusingNodeParamType.Enhanced Query Logic
Refactored the
node_listcommand insrc/aiida/cmdline/commands/cmd_node.pyto use a customQueryBuilder.This enables:
with_ancestorswith_descendantswith explicit tagging (
tag='node') to ensure correct filtering.Backward Compatibility
All existing features (sorting, projections, limits, and entry-point filters) remain unchanged.
Testing & Verification
1. Unit Tests
Added
tests/cmdline/commands/test_node_list_relationship.pywith test cases for:Result:
3 passed2. Manual Verification
Tested on a local SQLite profile using a simple provenance chain:
verdi node list --ancestor 3→ returns node 4verdi node list --descendant 4→ returns node 33. Regression Check
Executed
tests/cmdline/commands/test_node.pyto ensure no regressions.graphvizand prompt formatting), which are unrelated to this PR.