Skip to content
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

Docs: Node extras cannot be modified when iterating over QueryBuilder results with .iterall() #6009

Closed
GeigerJ2 opened this issue May 11, 2023 · 3 comments · Fixed by #6134
Closed

Comments

@GeigerJ2
Copy link
Contributor

Describe the current issue

Upon retrieving Nodes from the database with the QueryBuilder, iterating over them and changing the extras is not possible using .iterall(), but requires .all().

This might lead to confusion, as the .set_extra() method seems to work as expected, and the applied changes are reflected in the extras property of the Node instance during the iteration, but they are not actually written to the database.

Describe the solution you'd like

Considering that .iterall() returns a generator, this behavior is, of course, reasonable, so I don't think it warrants changes in the implementation. However, it might be helpful to add a note to the documentation (e.g. the "How to find and query for data" section) that no modifications to the Node extras can be made when using .iterall() or .iterdict().

@mbercx
Copy link
Member

mbercx commented May 18, 2023

Just to give a full example here: say you create a couple Int nodes in a fresh database:

from aiida import orm, load_profile
load_profile()

for i in range(5):
    node = orm.Int(i)
    node.store()

Then you add some extras using iterall

query = orm.QueryBuilder()
query.append(
    orm.Int
)
node_list = []
for [node] in query.iterall():
    node.base.extras.set('iterall', True)
    node_list.append(node)

You check that everything is ok and

[node.extras['iterall'] for node in node_list]

returns

[True, True, True, True, True]

You are overjoyed and continue your work. Later you restart the kernel and:

from aiida import orm, load_profile

load_profile()

query = orm.QueryBuilder()

query.append(
    orm.Int
)

[node.extras['iterall'] for node in query.all(flat=True)]

Fails this time with

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[1], line 11
      5 query = orm.QueryBuilder()
      7 query.append(
      8     orm.Int
      9 )
---> 11 [node.extras['iterall'] for node in query.all(flat=True)]

Cell In[1], line 11, in (.0)
      5 query = orm.QueryBuilder()
      7 query.append(
      8     orm.Int
      9 )
---> 11 [node.extras['iterall'] for node in query.all(flat=True)]

KeyError: 'iterall'

Much sadness. At some point you read the docstring of iterall and find:

        Same as :meth:`.all`, but returns a generator.
        Be aware that this is only safe if no commit will take place during this
        transaction. You might also want to read the SQLAlchemy documentation on
        https://docs.sqlalchemy.org/en/14/orm/query.html#sqlalchemy.orm.Query.yield_per

You check the link but don't immediately see anything that explains why no "commit" can take place in such an operation. Then you realise you don't really know enough about SQLalchemy to really understand any of this and you should probably find some time to change that.


More seriously, I'm not sure if a change to the docs is sufficient. I'm sure there are plenty of users that are not aware of this issue at all (I wasn't), and will have a very frustrating evening one day trying to figure out why their extras aren't there (I did). Probably should've checked the docstring more closely, but "Be aware that this is only safe if no commit will take place during this transaction." is also somewhat obscure. In the case of extras at lease, it seems safe, but just doesn't commit anything to the database.

I also don't really understand why the fact that iterall is a generator matters. But maybe someone more experienced with the database can explain that to me. ^^

@mbercx
Copy link
Member

mbercx commented May 19, 2023

ChatGPT has the answer. ^^

Screenshot 2023-05-19 at 02 16 15

@sphuber
Copy link
Contributor

sphuber commented Sep 24, 2023

See #6133 . Trying to submit a fix for this bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants