Improve the power managing actor initialization #966
Closed
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
When debugging and having some task in the stack coming from a `LastValueCache` it is very difficult to identify which one is it. This commit add a new `unique_id` that's used to create the task name. Signed-off-by: Leandro Lucarella <[email protected]>
The `str` just returns the value, and `repr` gets all the info, including the new `unique_id`. Signed-off-by: Leandro Lucarella <[email protected]>
This shouldn't be strictly necessary as the even loop only store weak references to tasks, and when a task is gone it should be removed from the event loop anyway, but this makes the process more explicit and might help cleaning up more properly if the task was still running when deleted, as the `CancelledError` might be still raised inside the task, giving it the opportunity to clean up. Signed-off-by: Leandro Lucarella <[email protected]>
This methods allows to wait until the cache is filled, to ensure working with a valid value at some particular point in time. Signed-off-by: Leandro Lucarella <[email protected]>
This method will wait that the manager actually received some data to ensure it can make informed decisions, and it is not just working on a partially initialized system. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Instead of blindly waiting for some time in the hopes that the underlying manager got some data, which can be a longer-than-needed wait when the data is available pretty quickly, we ask the manager to wait until the data is ready by using the new `wait_for_data()` method. Signed-off-by: Leandro Lucarella <[email protected]>
github-actions
bot
added
part:data-pipeline
|
@jh2007github FYI as it is (slightly) related to frequenz-floss/frequenz-core-python#7. |
llucax
added
the
cmd:skip-release-notes
|
Skipping release notes as the resulting effect should be barely noticeable for end users, but I can mention the improvement in the release notes if you prefer. |
|
The waiting for data at power distributor startup can be dropped instead: #971, so I guess some of the commits from this PR can go away. |
|
We leave the wait for data for |
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.
This PR was done while chasing an issue with tests complaining that some tasks were pending while the loop was destroyed.
I guess the issue is in the
pytest_asyncioplugin, because those tasks seem to be properly cancelled according to my debugging, and we had similar issues withpytest-asynciobefore. The weird thing is that the errors show up in a different test from the one that actually creates the tasks, which makes me think that the plugin is not properly cleaning up after itself.In any case, while debugging this, I found a few things that could be improved in the code in terms of synchoronization during initialization and cleanup.
This PR:
unique_idtoLatestValueCacheto improve the debugging experience (using the new nameunique_idinstead ofname, as suggested in RenameBackgroundService'snameargument tounique_idfrequenz-core-python#7).__str__and__repr__toLatestValueCacheto use thisunique_idLatestValueCache's task if deleted (just in case)wait_for_value()method toLatestValueCacheso we can wait for the cache to be filled with datawait_for_data()method toComponentManagerto wait for all caches to be filled with data@overridetoPowerDistributingActor(because why not?)PowerDistributingActorby using the newwait_for_data()method instead of blidly wait for the whole wait time