-
Notifications
You must be signed in to change notification settings - Fork 239
Issue #6687 Fix QueryBuilder filtering for AbstractCode #6786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
danielhollas
left a comment
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.
Thank you for the contribution @adityagh006! 👏
This will need tests, which should cover various types of codes as described in the issue. Hopefully the existing QueryBuilder tests in aiida-core/tests/orm/test_querybuilder.py can help you get started. Let us now if you need assistance!
src/aiida/orm/querybuilder.py
Outdated
| # value = classifiers.ormclass_type_string | ||
|
|
||
| # if not subclassing: | ||
| # filters = {'==': value} | ||
| # else: | ||
| # # Note: the query_type_string always ends with a dot. This ensures that "like {str}%" matches *only* | ||
| # # the query type string | ||
| # filters = {'like': f'{escape_for_sql_like(get_query_type_from_type_string(value))}%'} | ||
|
|
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.
Please remove all commented out code (also below)
| # value = classifiers.ormclass_type_string | |
| # if not subclassing: | |
| # filters = {'==': value} | |
| # else: | |
| # # Note: the query_type_string always ends with a dot. This ensures that "like {str}%" matches *only* | |
| # # the query type string | |
| # filters = {'like': f'{escape_for_sql_like(get_query_type_from_type_string(value))}%'} |
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.
Oh, sorry for that! I forgot to remove those comments. I'll take care of it in the next update.
src/aiida/orm/querybuilder.py
Outdated
|
|
||
| value = classifiers.ormclass_type_string | ||
|
|
||
| if value == 'data.core.code.abstract': |
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 think this needs a longer comment to explain this corner case. You can perhaps reuse part of the explanation from the issue.
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.
Got it! I'll add a detailed comment explaining this corner case, incorporating relevant details from the issue.
|
Hi @adityagh006, are you still planning to work on this PR? I think the code looks good (though I haven't tested it personally) but it needs unit tests. |
|
@danielhollas! I’ve written some test cases for the AbstractCode filtering, but I’m having trouble getting them to run properly. I’ve tried a few things, but I'm not sure if it’s an environment issue or something else—I’m a bit stuck at the moment. |
|
@adityagh006 no worries. Feel free to push what you have and I can have a look. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6786 +/- ##
==========================================
- Coverage 78.31% 78.31% -0.00%
==========================================
Files 566 566
Lines 42762 42764 +2
==========================================
+ Hits 33484 33485 +1
- Misses 9278 9279 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@danielhollas! I’m currently caught up with a college assignment, but I’ll make sure to push the test cases by end of day tomorrow. |
607c854 to
ac57010
Compare
danielhollas
left a comment
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.
@adityagh006 can you please rebase this PR on latest main? It now contains some extra commits.
The logic of the new tests looks good! Just needs a couple of fixes, see my comments. Let me know if you need further help, and thanks for working through this!
tests/orm/test_querybuilder.py
Outdated
|
|
||
| portable_code = orm.PortableCode( | ||
| label='test_portable_abstract', | ||
| filepath_executable='/bin/bash', |
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 think this needs to be fixed, please look at existing tests in tests/orm/data/code/test_portable.py
| filepath_executable='/bin/bash', | |
| filepath_paths='/bin/bash', |
tests/orm/test_querybuilder.py
Outdated
| when looking for AbstractCode due to a node_type mismatch. | ||
| """ | ||
| # Set up test environment | ||
| computer = orm.Computer( |
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.
Please use an existing fixture aiida_computer_local to simplify this. You can then delete the cleanup code since the fixture does that automatically at the end of the test.
tests/orm/test_querybuilder.py
Outdated
| qb = orm.QueryBuilder().append(orm.Data, filters={'or': [{}, {}]}) | ||
| assert qb.count() == count | ||
|
|
||
| from aiida.orm import AbstractCode, Computer, InstalledCode, PortableCode, QueryBuilder |
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.
Let's move these imports at the top of the file
tests/orm/test_querybuilder.py
Outdated
| # Test 1: AbstractCode query should find all code types | ||
| qb_abstract = orm.QueryBuilder().append(orm.AbstractCode) | ||
| abstract_results = qb_abstract.all(flat=True) | ||
| assert installed_code in abstract_results, 'InstalledCode not found with AbstractCode query' |
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 don't think these asserts will work, since the objects returned by the QueryBuilder will be different from the original objects that you stored, even though they point to the same DB node. I think you should compare the id of the nodes, something like
| assert installed_code in abstract_results, 'InstalledCode not found with AbstractCode query' | |
| abstract_ids = [code.id for code in abstract_results] | |
| assert installed_code.id in abstract_ids, 'InstalledCode not found with AbstractCode query' |
pyproject.toml
Outdated
| 'process.workflow.workfunction' = 'aiida.orm.nodes.process.workflow.workfunction:WorkFunctionNode' | ||
|
|
||
| [project.entry-points.'aiida.orm'] | ||
| 'core.code.abstract' = 'aiida.orm.nodes.data.code.abstract:AbstractCode' |
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 think this entry point should be moved to [project.entry-points.'aiida.data'] section where the other code entry points are defined.
pyproject.toml
Outdated
| [project.entry-points.'aiida.workflows'] | ||
| 'core.arithmetic.add_multiply' = 'aiida.workflows.arithmetic.add_multiply:add_multiply' | ||
| 'core.arithmetic.multiply_add' = 'aiida.workflows.arithmetic.multiply_add:MultiplyAddWorkChain' | ||
| 'core.code.abstract' = 'aiida.orm.nodes.data.code.abstract:AbstractCode' |
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.
| 'core.code.abstract' = 'aiida.orm.nodes.data.code.abstract:AbstractCode' |
|
Hi @danielhollas, Apologies for the inconvenience. |
|
@adityagh006 feel free to open a new one. I can help with the rebase, but there are currently no commits in this branch so I cannot... |
|
Hi @danielhollas, Apologies for the delay, but I’ve created a new PR for the changes. You can find it here. Let me know if any further adjustments are needed. |
|
Hi @danielhollas, |
This PR fixes an issue in QueryBuilder where filtering for AbstractCode was not working correctly. The update ensures that queries involving AbstractCode return expected results.
Changes Made:
1.Fixed filtering logic in querybuilder.py.
2.Updated pyproject.toml accordingly.
Let me know if any additional changes are needed.