ORM: Cache the logger adapter for ProcessNode
#6492
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
khsrali
reviewed
Jun 28, 2024
| @@ -162,6 +162,7 @@ class ProcessNode(Sealable, Node): | |||
| METADATA_INPUTS_KEY: str = 'metadata_inputs' | |||
|
|
|||
| _unstorable_message = 'only Data, WorkflowNode, CalculationNode or their subclasses can be stored' | |||
| _logger_adapter = None | |||
There was a problem hiding this comment.
is this a class variable or an instance variable?
There was a problem hiding this comment.
It is a class variable, but you are right it should be an instance variable. I have updated the code.
There was a problem hiding this comment.
@khsrali Ok, it turned out it was a bit more difficult. We cannot initialize the _logger_adapter in the constructor, because that path is not followded when loading an existing node from the database. So we have initialize it in the logger property and initialize it as None as a class variable. Since logger sets the adapter on the instance, it won't pollute the class variable though, so it seems fine as a solution.
sphuber
force-pushed
the
feature/cache-process-node-logger-adapter
branch
4 times, most recently
from
80eb6bc
to
194abbc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6492 +/- ##
==========================================
+ Coverage 77.51% 77.79% +0.29%
==========================================
Files 560 561 +1
Lines 41444 41809 +365
==========================================
+ Hits 32120 32521 +401
+ Misses 9324 9288 -36 ☔ View full report in Codecov by Sentry. |
sphuber
force-pushed
the
feature/cache-process-node-logger-adapter
branch
from
194abbc
to
ea88d34
Compare
khsrali
reviewed
Jun 29, 2024
| @@ -251,7 +259,16 @@ def logger(self): | |||
| """ | |||
| from aiida.orm.utils.log import create_logger_adapter | |||
|
|
|||
| return create_logger_adapter(self._logger, self) | |||
| # If the node is not yet stored, there is no point in creating the logger adapter yet as the ``DbLogHandler`` | |||
| # it configures only is triggered for stored nodes otherwise it cannot link the log message to the node. | |||
There was a problem hiding this comment.
maybe consider rephrasing # it configures only is triggered
The logger adapter was recreated each time the `logger` property of the `ProcessNode` was invoked. It is now created once in the `logger` property. The created logger adapter is assigned to the `_logger_adapter` attribute such that it can simply be returned at the next invocation. The initialization of the adapter cannot be done in the constructor as that route is not taken if an existing node is loaded from the database. Finally, the `logger` property only creates and returns the adapter when the node is stored. Otherwise it simply returns the base logger instance. This is because the logger adapter only works for stored nodes and if it were instantiated at the point when the node is unstored, it would not be regenerated once the node is stored, and so the `DbLogHandler` will never be able to persist log messages to the database.
sphuber
force-pushed
the
feature/cache-process-node-logger-adapter
branch
from
ea88d34
to
1e81fde
Compare
sphuber
force-pushed
the
feature/cache-process-node-logger-adapter
branch
from
1e81fde
to
7bb9e62
Compare
khsrali
reviewed
Jun 29, 2024
khsrali
approved these changes
Jun 29, 2024
Co-authored-by: Ali Khosravi <khsrali@gmail.com>
for more information, see https://pre-commit.ci
mikibonacci
pushed a commit
to mikibonacci/aiida-core
that referenced
this pull request
Sep 3, 2024
The logger adapter was recreated each time the `logger` property of the `ProcessNode` was invoked. It is now created once in the `logger` property. The created logger adapter is assigned to the `_logger_adapter` attribute such that it can simply be returned at the next invocation. The initialization of the adapter cannot be done in the constructor as that route is not taken if an existing node is loaded from the database. Finally, the `logger` property only creates and returns the adapter when the node is stored. Otherwise it simply returns the base logger instance. This is because the logger adapter only works for stored nodes and if it were instantiated at the point when the node is unstored, it would not be regenerated once the node is stored, and so the `DbLogHandler` will never be able to persist log messages to the database.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The logger adapter was recreated each time the
loggerproperty of theProcessNodewas invoked. It is now cached on the node instance as its contents should not change for the lifetime of the node.