Skip to content

Clean ups and improvements to the resampler (mostly) #68

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Nov 21, 2022
Merged

Clean ups and improvements to the resampler (mostly) #68

merged 20 commits into from
Nov 21, 2022

Conversation

@leandro-lucarella-frequenz
Copy link
Contributor

@leandro-lucarella-frequenz leandro-lucarella-frequenz commented Nov 16, 2022

This PR mainly moves classes from the resampler to the new timeseries
package, but it also includes some nox and test improvements and fixes.

It also uses the timestamp produced by the timer as the resampling timestamp,
unifying it across all the resampling levels for full consistency.

  • Allow passing positional arguments to nox sessions
  • Move resampling actor to frequenz.sdk.actor
  • Move resampler classes to frequenz.sdk.timeseries.resampler
  • Remove unnecessary usage of pytz
  • Fix sample ordering in tests
  • Use more intuitive variable names in resampler tests
  • Use easier to debug fake times in tests
  • Allow the resampler to use an external timestamp
  • Use the timestamp provided by the timer receiver
  • Document ResamplingFunction
  • Move Sample to frequenz.sdk.timeseries

@leandro-lucarella-frequenz leandro-lucarella-frequenz requested a review from a team as a code owner November 16, 2022 13:21
@leandro-lucarella-frequenz leandro-lucarella-frequenz added this to the v0.12.0 milestone Nov 16, 2022
@leandro-lucarella-frequenz leandro-lucarella-frequenz added priority:high Address this as soon as possible type:bug Something isn't working type:enhancement New feature or enhancement visitble to users type:tech-debt Improves the project without visible changes for users labels Nov 16, 2022
@github-actions github-actions bot added part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline part:power-management Affects the management of battery power and distribution part:tests Affects the unit, integration and performance (benchmarks) tests labels Nov 16, 2022
@leandro-lucarella-frequenz
Copy link
Contributor Author

leandro-lucarella-frequenz commented Nov 16, 2022

This might depend on frequenz-floss/frequenz-channels-python#48, I am getting a lot of flakyness in tests, stalling at the grpc mocking layer. At some point the traceback showed issues with comparing time aware and native datetimes, but locally I still get sometimes stalled tests. It is very hard to find a way to reproduce and it doesn't always seem to be related to these changes, but it happens even when rebuilding the whole virtualenv.

@leandro-lucarella-frequenz leandro-lucarella-frequenz linked an issue Nov 16, 2022 that may be closed by this pull request
Use the datetime produced by the timer in the resampling actor #59
Closed
@leandro-lucarella-frequenz
Copy link
Contributor Author

leandro-lucarella-frequenz commented Nov 16, 2022

This is also a step forward (needed preparation) for making the API more stable and for #56.

@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Nov 17, 2022
@leandro-lucarella-frequenz
Copy link
Contributor Author

leandro-lucarella-frequenz commented Nov 17, 2022

Updated:

  • Added a few more cleanups (remove pytz)
  • Minimized the flakiness of the resampling test, I had a double-failure, both the channels "native" datetime and the intrinsic flakiness of the tests were making my life debugging misserable
  • Added a temporary commit to use frequenz-channels v0.x.x to config the fix works (and it does 🎉)

@shsms shsms mentioned this pull request Nov 17, 2022
Merged
@leandro-lucarella-frequenz leandro-lucarella-frequenz linked an issue Nov 18, 2022 that may be closed by this pull request
Document the ResamplingFunction #60
Closed
@llucax
Allow passing positional arguments to nox sessions
5862456 
All sessions can now take positional arguments, which will be forwarded
to the command run by the session. This is usually very useful to check
individual files, or pass extra arguments, like:

nox -R -s pytest_max -- -s tests/my/test_this.py

Sessions will not process the pre-defined files when positional
arguments are passed, so when using positional arguments you should
always include paths to check, except for pytest_xxx sessions, which
still if files are omitted, all files will be processed, so it can be
used to use custom options only too. For example:

nox -R -s pytest_max -- -sx

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move resampling actor to frequenz.sdk.actor
b47602a 
More precisely, from:
frequenz.sdk.data_ingestion.resampling.component_metrics_resampling_actor
to: frequenz.sdk.actor.resampling

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move resampler classes to frequenz.sdk.timeseries.resampler
c71ed5f 
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Remove unnecessary usage of pytz
d229088 
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Fix sample ordering in tests
c869167 
The resampler expects samples to be in order (from older to newer),
otherwise when calculating which samples are relevant to the current
resampling window some valid samples could be left out.

Some tests didn't have them in the proper order but they were working
just by chance.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Use more intuitive variable names in resampler tests
e2136d1 
With these names it is easier to identify which sample belongs to each
metric.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Use easier to debug fake times in tests
9960656 
We use the epoch so it is very easy to debug time issues when using
datetime.timestamp().

We also stop the time from ticking, because we want to have full control
over time passing in these tests.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Allow the resampler to use an external timestamp
639961d 
This is necessary to achieve full timestamp consistency across the whole
resampling machinery, otherwise there could be some inconsistencies when
calculating which are the current relevant samples to use when
resampling in the ComponentMetricResampler.

The ComponentMetricGroupResampler now passes the timestamp to all the
ComponentMetricResampler it controls.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Use the timestamp provided by the timer receiver
00485f7 
The resampling class now can take an optional timestamp to be used to
produce the new samples and the resampling actor now passes the
timestamp produced by the timer instead of calling `now()` manually.

This also adds a more explicit check to make sure the timer receiver is
not closed.

Fixes frequenz-floss#54.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Document ResamplingFunction
5821901 
There is some redundancy when specifying the types for arguments and the
return value because we are going to use mkdocstrings and it doesn't
support this very specific use case of documenting a callable type
specifically.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move Sample to frequenz.sdk.timeseries
908a0b3 
Now that we have a timeseries package it makes more sense to have the
Sample class living there.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Remove dependency on pytz
38c70ed 
All we are using pytz for is to using aware datetime object with UTC
timezone, which is perfectly doable with the standard datetime and
timezone objects.

This commit also converts some instances of "native" datetimes to
"aware".

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Properly stop server in test if an assertion fails
344bd23 
If an assertion is raised and we don't properly stop the mock server,
the test stalls waiting for the server to finish.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Fix formatting using black
00d3e82 
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Minimize flakiness and simplify test
95cf692 
The resampling actor test was very complex, including starting a mock
gRPC server. This adds a lot of complexity to debug the test when it
fails and adds a lot of unnecessary complexity to the test and mocking
needs (we need to mock components in the component graph and interact
with the microgrid API when the resampling actor knows nothing about the
microgrid API).

To completely remove the flakiness, we need to use some solution to mock
time in asyncio (see frequenz-floss#70).

Signed-off-by: Leandro Lucarella <[email protected]>
sahas-subramanian-frequenz
sahas-subramanian-frequenz previously approved these changes Nov 18, 2022
View reviewed changes
Copy link
Contributor

@sahas-subramanian-frequenz sahas-subramanian-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. There's a commit updating channel version to the dev version. Do you want to remove that and take this in, and update channel version once there is a new release?

@leandro-lucarella-frequenz
Copy link
Contributor Author

leandro-lucarella-frequenz commented Nov 21, 2022

This LGTM. There's a commit updating channel version to the dev version. Do you want to remove that and take this in, and update channel version once there is a new release?

I'll wait and see if we can make the channels release soon enough.

@llucax
Use development version of frequenz-channels
3097fd2 
We will switch back to a release when the new release is ready.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Rename ComponentMetric(Group)Resampler to Resampler
f3de661 
Both ComponentMetricGroupResampler and ComponentMetricResampler have
nothing to do with component metrics, so it makes no sense to have that
in the name.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Make the resampler module private
e519a7a 
The resampler classes are already exposed via the `timeseries` package,
so there is no need to have them exposed twice.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Use relative imports
479ec00 
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move sdk.data_pipeline to sdk.actor.data_sourcing
a62350c 
All the types left in `frequenz.sdk.data_pipeline(.types)` were specific
to the data sourcing actor, so they were moved to
`frequenz.sdk.data_sourcing.types` (and exported in
`frequenz.sdk.data_sourcing`).

The resampling actor for now is also forwarding requests to the data
sourcing actor, so it also re-exports those symbols.

Signed-off-by: Leandro Lucarella <[email protected]>
@leandro-lucarella-frequenz
Copy link
Contributor Author

leandro-lucarella-frequenz commented Nov 21, 2022

Updated to move the data_pipeline stuff to the data sourcing actor (and make some imports relative). I also renamed the commit to use the devel channels to remove the TMP: , I think we should merge this as is for now to unblock other PRs that might be better to rebase on top of this one. I created #71.

@leandro-lucarella-frequenz leandro-lucarella-frequenz merged commit 0cb59e1 into frequenz-floss:v0.x.x Nov 21, 2022
@leandro-lucarella-frequenz leandro-lucarella-frequenz deleted the resample-cleanup branch November 21, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ela-kotulska-frequenz ela-kotulska-frequenz Awaiting requested review from ela-kotulska-frequenz ela-kotulska-frequenz was automatically assigned from frequenz-floss/python-sdk-team

1 more reviewer

@sahas-subramanian-frequenz sahas-subramanian-frequenz sahas-subramanian-frequenz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@leandro-lucarella-frequenz leandro-lucarella-frequenz

Labels

part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline part:power-management Affects the management of battery power and distribution part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) priority:high Address this as soon as possible type:bug Something isn't working type:enhancement New feature or enhancement visitble to users type:tech-debt Improves the project without visible changes for users

Projects

Python SDK Roadmap
Status: Done

Milestone

v0.12.0

Development

Successfully merging this pull request may close these issues.

Document the ResamplingFunction Use the datetime produced by the timer in the resampling actor

3 participants

@leandro-lucarella-frequenz @sahas-subramanian-frequenz @llucax