node.open: more useful warnings #4434

ltalirz · 2020-10-10T21:15:04Z

aiida-core started warning when methods like Node.read and Node.del
are used outside a context manager.
The warning, however, did not include any indication of where the
problematic function call was made (nor did it include instructions on
how to fix it).

Furthermore, this PR fixes an overlooked use of Node.read in
aiida-core that did not rely on the context manager.

ltalirz · 2020-10-10T21:20:16Z

Example warning:

In [1]: n = load_node(123)

In [2]: n.open('aiida.out').read()[1]
/Users/leopold/Personal/Postdoc-MARVEL/repos/aiida/aiida_rmq/aiida/orm/nodes/node.py:64: AiidaDeprecationWarning:
`Node.read` used outside context manager for <FolderData: uuid: 2146a20c-193a-4e76-b492-f868a8cf31a0 (pk: 123)>.
Please use `with <node>.open(): ...` instead. This will raise starting from `aiida-core==2.0.0`
File <ipython-input-2-6452f92039da>, line 1 in module <module>  File "<ipython-input-2-6452f92039da>", line 1, in <module>
    n.open('aiida.out').read()[1]

  warnings.warn(msg, AiidaDeprecationWarning)  # pylint: disable=no-member
/Users/leopold/Personal/Postdoc-MARVEL/repos/aiida/aiida_rmq/aiida/orm/nodes/node.py:64: AiidaDeprecationWarning:
`Node.del` used outside context manager for <FolderData: uuid: 2146a20c-193a-4e76-b492-f868a8cf31a0 (pk: 123)>.
Please use `with <node>.open(): ...` instead. This will raise starting from `aiida-core==2.0.0`
File <ipython-input-2-6452f92039da>, line 1 in module <module>  File "<ipython-input-2-6452f92039da>", line 1, in <module>
    n.open('aiida.out').read()[1]

  warnings.warn(msg, AiidaDeprecationWarning)  # pylint: disable=no-member

It's a bit ugly that traceback.StackSummary.from_list(caller_traceback).format() adds the blank line.
I could also use inspect.getframeinfo and then essentially recreate

File <ipython-input-2-6452f92039da>, line 1 in module <module>  File "<ipython-input-2-6452f92039da>", line 1, in <module>
    n.open('aiida.out').read()[1]

from this but that also seemed like overkill.
Let me know in case you know a better way (maybe I'm overlooking some obvious built-in function here)

codecov · 2020-10-10T21:31:00Z

Codecov Report

Merging #4434 into develop will decrease coverage by 0.01%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           develop    #4434      +/-   ##
===========================================
- Coverage    79.36%   79.35%   -0.00%     
===========================================
  Files          476      476              
  Lines        34951    34959       +8     
===========================================
+ Hits         27736    27739       +3     
- Misses        7215     7220       +5

Flag	Coverage Δ
#django	`73.22% <80.00%> (-<0.01%)`	⬇️
#sqlalchemy	`72.45% <80.00%> (-<0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/orm/nodes/node.py	`81.15% <71.43%> (-0.66%)`	⬇️
aiida/orm/nodes/data/cif.py	`86.65% <100.00%> (+0.08%)`	⬆️
aiida/engine/daemon/client.py	`72.42% <0.00%> (-1.14%)`	⬇️
aiida/transports/plugins/local.py	`82.83% <0.00%> (+0.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c49471...cea718b. Read the comment docs.

greschd · 2020-10-11T09:50:18Z

aiida/orm/nodes/node.py

+            msg = f'\n`Node.{method}` used outside context manager for {self._name}.\n' + \
+                  'Please use `with <node>.open(): ...` instead. ' + \
+                  'This will raise starting from `aiida-core==2.0.0`\n'
+            caller_frame = inspect.currentframe().f_back.f_back


I wonder if there is a possibility that this code fails - maybe we can just wrap it in a try / except such that it fails gracefully?

Done (for the case when f_back returns None

sphuber · 2020-10-11T13:46:12Z

aiida/orm/nodes/node.py

-            msg = '`{}` used without context manager for {}. This will raise starting from `aiida-core==2.0.0`'.format(
-                method, self._name
-            )
+            msg = f'\n`Node.{method}` used outside context manager for {self._name}.\n' + \


The Node.{method} is not actually correct and may be misleading. The method here is what is actually called on the return value of Node.open. Normally this is a filelike object, but now it is this instance of WarnWhenNotEntered that we added. You can see this when this object is garbage collected, because it will then say Node.del used outside .. but Node doesn't have a del method. Same goes for read though.

Consider following example:

handle = node.open() content = handle.read()

It is when read is called on handle that we get the warning, not on the node. Likewise, it is del handle that is called when it is garbage collected that incurs the second warning.

This shows another potential problem, with the example above, the line of code that you will include in your warning will be content = handle.read() and people may be confused by you to tell them that this should be with node.open(). Note that in this case, the fix may be obvious, because they will go to the line mentioned, and see the node.open() call straight above it. However, they are not necessarily very close. They could have passed the handle to some function, so then it may not be clear at all where the actual problem is.

The Node.{method} is not actually correct and may be misleading. The method here is what is actually called on the return value of Node.open.

Right, I've now tried to rephrase the message to make this clearer. Further suggestions welcome.

the line of code that you will include in your warning will be content = handle.read() and people may be confused by you to tell them that this should be with node.open()

You are very welcome to suggest a better way.
In my view, this warning already improves the situation enormously, since it shows which call caused the warning rather than where the warning is defined.
For me this is good enough.

sphuber · 2020-10-11T13:47:14Z

aiida/orm/nodes/node.py

+                  'This will raise starting from `aiida-core==2.0.0`\n'
+            caller_frame = inspect.currentframe().f_back.f_back
+            caller_tb = inspect.getframeinfo(caller_frame)
+            msg += f'File {caller_tb.filename}, line {caller_tb.lineno} in module {caller_tb.function}'


Not sure why, but I see the line with File {caller_tb.filename} .. twice

/home/sph/code/aiida/env/dev/aiida-core/aiida/orm/nodes/node.py:62: AiidaDeprecationWarning: `Node.read` used outside context manager for <ForceConstantsData: uuid: 6444b4db-4c69-484d-87d1-5ab6d38d8e46 (unstored)>. Please use `with <node>.open(): ...` instead. This will raise starting from `aiida-core==2.0.0` File /home/sph/code/aiida/env/dev/aiida-quantumespresso/aiida_quantumespresso/data/force_constants.py, line 22 in module set_file File "/home/sph/code/aiida/env/dev/aiida-quantumespresso/aiida_quantumespresso/data/force_constants.py", line 22, in set_file content = handle.read().splitlines() warnings.warn(msg, AiidaDeprecationWarning) # pylint: disable=no-member

Any idea why?

Sorry, this didn't show up for me since I was working inside the ipython shell, and so I thought I had to recreate this line manually.
I've removed it now; should be fixed.

sphuber · 2020-10-26T17:37:47Z

@ltalirz I think it would be good to try and get this into the v1.5.0 release as well. Do you have the time to still take a look at this?

ltalirz · 2020-10-26T17:45:34Z

Yes

ltalirz · 2020-10-26T17:46:41Z

@sphuber Could you please check #4393 and #4511 ?

sphuber · 2020-10-26T19:17:30Z

aiida/orm/nodes/node.py

+            msg = f'\n`{method}` called outside Node.open() context manager for {self._name}.\n' + \
+                  'Please wrap this call inside `with <node instance>.open(): ...` to silence this warning. ' + \
+                  'This will raise starting from `aiida-core==2.0.0`\n'
+            try:
+                caller_frame = inspect.currentframe().f_back.f_back
+            except AttributeError:
+                warnings.warn(msg, AiidaDeprecationWarning)
+                return
+
+            caller_traceback = traceback.extract_stack(caller_frame, limit=1)
+            for item in traceback.StackSummary.from_list(caller_traceback).format():
+                msg += item
            warnings.warn(msg, AiidaDeprecationWarning)  # pylint: disable=no-member


I think putting the one call in try/except helps but is potentially not enough. We really want to avoid exceptions occurring in this handler. I also think the code to extract the relevant line of code can be much simpler. I propose the following for the entire method, which has updated warning message and stack extraction:

if not self._was_entered: msg = f'\nThe method `{method}` was called on the return value of `{self._name}.open()` outside of a context manager.\n' + \ 'Please wrap this call inside `with <node instance>.open(): ...` to silence this warning. ' + \ 'This will raise an exception, starting from `aiida-core==2.0.0`.\n' try: caller = traceback.format_stack()[-3] except Exception: # pylint: disable=broad-except msg += 'Could not determine the line of code responsible for triggering this warning.' else: msg += f'The offending call comes from:\n{caller}' warnings.warn(msg, AiidaDeprecationWarning) # pylint: disable=no-member

sounds good to me!

aiida-core started warning when methods like `Node.read` and `Node.del` are used outside a context manager. The warning, however, did not include any indication of where the problematic function call was made (nor did it include instructions on how to fix it). Furthermore, this PR fixes an overlooked use of `Node.read` in aiida-core that did not rely on the context manager.

sphuber

Thanks @ltalirz

ltalirz force-pushed the issue_4335_deprecation_warning branch from a6c0f11 to ac1479a Compare October 10, 2020 21:16

ltalirz mentioned this pull request Oct 10, 2020

Prepare the code for the new repository implementation #4344

Merged

greschd reviewed Oct 11, 2020

View reviewed changes

sphuber self-requested a review October 11, 2020 13:38

sphuber reviewed Oct 11, 2020

View reviewed changes

ltalirz force-pushed the issue_4335_deprecation_warning branch from ac1479a to cf99009 Compare October 26, 2020 18:14

ltalirz requested a review from sphuber October 26, 2020 18:17

ltalirz force-pushed the issue_4335_deprecation_warning branch from cf99009 to fdbe999 Compare October 26, 2020 18:18

sphuber reviewed Oct 26, 2020

View reviewed changes

ltalirz force-pushed the issue_4335_deprecation_warning branch from fdbe999 to ef4ddbf Compare October 26, 2020 19:21

ltalirz force-pushed the issue_4335_deprecation_warning branch from ef4ddbf to a7f4867 Compare October 26, 2020 19:45

ltalirz requested a review from sphuber October 26, 2020 23:15

Merge branch 'develop' into issue_4335_deprecation_warning

cea718b

sphuber approved these changes Oct 27, 2020

View reviewed changes

sphuber merged commit 57c8afa into aiidateam:develop Oct 27, 2020

sphuber deleted the issue_4335_deprecation_warning branch October 27, 2020 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node.open: more useful warnings #4434

node.open: more useful warnings #4434

ltalirz commented Oct 10, 2020

ltalirz commented Oct 10, 2020 •

edited

Loading

codecov bot commented Oct 10, 2020 •

edited

Loading

greschd Oct 11, 2020

ltalirz Oct 26, 2020

sphuber Oct 11, 2020

ltalirz Oct 26, 2020

sphuber Oct 11, 2020

ltalirz Oct 26, 2020 •

edited

Loading

sphuber commented Oct 26, 2020

ltalirz commented Oct 26, 2020

ltalirz commented Oct 26, 2020 •

edited

Loading

sphuber Oct 26, 2020

ltalirz Oct 26, 2020

sphuber left a comment

node.open: more useful warnings #4434

node.open: more useful warnings #4434

Conversation

ltalirz commented Oct 10, 2020

ltalirz commented Oct 10, 2020 • edited Loading

codecov bot commented Oct 10, 2020 • edited Loading

Codecov Report

greschd Oct 11, 2020

Choose a reason for hiding this comment

ltalirz Oct 26, 2020

Choose a reason for hiding this comment

sphuber Oct 11, 2020

Choose a reason for hiding this comment

ltalirz Oct 26, 2020

Choose a reason for hiding this comment

sphuber Oct 11, 2020

Choose a reason for hiding this comment

ltalirz Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

sphuber commented Oct 26, 2020

ltalirz commented Oct 26, 2020

ltalirz commented Oct 26, 2020 • edited Loading

sphuber Oct 26, 2020

Choose a reason for hiding this comment

ltalirz Oct 26, 2020

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

ltalirz commented Oct 10, 2020 •

edited

Loading

codecov bot commented Oct 10, 2020 •

edited

Loading

ltalirz Oct 26, 2020 •

edited

Loading

ltalirz commented Oct 26, 2020 •

edited

Loading