[0.3.2] Replace start timestamp with number of observations #1402

Adamantios · 2022-09-28T15:21:19Z

Proposed changes

Replace the start timestamp in the configuration with the number of observations we want for the timeseries.
We now have to set the number of data points we wish to download in the past (starting from the configured end, using the configured interval).
This is more intuitive than setting the exact start timestamp in UNIX, resulting in better DevX.

Fixes

n/a

Types of changes

What types of changes does your code introduce? (A breaking change is a fix or feature that would cause existing functionality and APIs to not work as expected.)
Put an x in the box that applies

Non-breaking fix (non-breaking change which fixes an issue)
Breaking fix (breaking change which fixes an issue)
Non-breaking feature (non-breaking change which adds functionality)
Breaking feature (breaking change which adds functionality)
Refactor (non-breaking change which changes implementation)
Messy (mixture of the above - requires explanation!)

Checklist

Put an x in the boxes that apply.

I have read the CONTRIBUTING doc
I am making a pull request against the main branch (left side). Also you should start your branch off our main.
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have locally run services that could be impacted and they do not present failures derived from my changes

Further comments

n/a

This results in better DevX. It is more intuitive to set the number of observations than the exact date.

Closes #1396.

Windows tests occasionally timeout.

Adjust APY models tests to the refactoring which replaced the `history_start` with the `n_observations`. Refactoring commits: - 4a410913594dd6088494b4e1381f355233d2f8d8 - cc1620e1b6e11c5e339fdbfe6bc2e65544fa8aac - d6488fe14c60b3dce44675a8611c47c0cac4c0f7

cc1620e1b6e11c5e339fdbfe6bc2e65544fa8aac

Adamantios · 2022-09-28T15:22:35Z

.github/workflows/main_workflow.yml

-    timeout-minutes: 65
+    timeout-minutes: 70


Increased timeout as I have noticed windows tests timing out lately and I also added two new hypothesis tests.

Adamantios · 2022-09-28T15:24:56Z

packages/valory/services/apy_estimation/service.yaml

-      history_start: ${SERVICE_APY_HISTORY_START:int:null}
+      n_observations: ${SERVICE_APY_N_OBSERVATIONS:int:120}


Set a default of 120 observations which is a good number to start experimenting with. Moreover, now the operator does not need to specify anything if they do not need to update the number of observations. Previously, they had to update the history_start every time they restarted the service which was cumbersome.

yes. There probably is some range that should be allowed, not up to infinity

In aea-config.yaml n_observations is 10 while here it is 120. Is that right?

Agreed. I plan to extend this method at some point, though we might want to introduce a proper way for validation to all of our models:

open-autonomy/packages/valory/skills/apy_estimation_abci/models.py

Lines 209 to 220 in 554269b

def __validate_params(self) -> None:

"""Validate the given parameters."""

# Eventually, we should probably validate all the parameters. E.g., `ts_length` should be < `end`

for param_name in ("timeout", "window_size"):

param_val = self.optimizer_params.get(param_name)

if param_val is not None and not isinstance(param_val, int):

raise ValueError(

f"Optimizer's parameter `{param_name}` can be either of type `int` or `None`. "

f"{type(param_val)} was given."

)

# if the value did not exist in the config, then we set it to the default (None) returned from `.get` method

self.optimizer_params[param_name] = param_val

Have you heard of pydantic? Using it might produce some cool results.

In aea-config.yaml n_observations is 10 while here it is 120. Is that right?

Yes, we need the service to run with proper values and the CI to run fast.

Adamantios · 2022-09-28T15:25:54Z

packages/valory/skills/apy_estimation_abci/behaviours.py

-            unit_amount_from_sec(self.params.end - self.params.start, self._unit)
+            unit_amount_from_sec(self.params.ts_length, self._unit)


We now have a property for this:

open-autonomy/packages/valory/skills/apy_estimation_abci/models.py

Lines 204 to 207 in 554269b

@property

def ts_length(self) -> int:

"""The length of the timeseries in seconds."""

return self.n_observations * self.interval

Adamantios · 2022-09-28T15:26:30Z

packages/valory/skills/apy_estimation_abci/behaviours.py

            self.context.logger.info(
                "Estimates have been received:\n" f"{estimates.to_string()}"
            )
-            self.context.logger.info("Estimates have been received.")



Removed because it was duplicate.

Adamantios · 2022-09-28T15:28:01Z

packages/valory/skills/apy_estimation_abci/models.py

@@ -192,9 +194,21 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:

        self.__validate_params()

+    @property
+    def start(self) -> Optional[int]:


start is now given by a property that utilizes the ts_length. The formula is:

start = end - n_observations * interval

end and interval are given in UNIX, and therefore start is also in UNIX.

Adamantios · 2022-09-28T15:30:32Z

plugins/aea-test-autonomy/aea_test_autonomy/fixture_helpers.py

-    max_attempts: int = 20,
+    max_attempts: int = 26,


Addresses #1396. 12 extra seconds should be enough for CI.

ok so that's why. It's an odd number to pick though

I think it's an even 😅 🤣

yeah not a prime number 👎

docs/api/plugins/aea_test_autonomy/fixture_helpers.md

packages/valory/skills/apy_estimation_abci/tests/test_models.py

Karrenbelt · 2022-09-28T15:39:26Z

packages/valory/skills/apy_estimation_abci/tests/test_behaviours.py

+        # filter out end values that will result in negative `start`
+        assume(end >= n_observations * interval)
+


is this better or somehow different than than using .filter(lambda a, b, c: c >= a * b)?

I think that they both filter the values, and used assume as I found it easier to read.

packages/valory/skills/apy_estimation_abci/tests/test_behaviours.py

codecov · 2022-09-28T16:07:41Z

Codecov Report

Base: 92.70% // Head: 92.70% // No change to project coverage 👍

Coverage data is based on head (e8fd983) compared to base (b154e1c).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1402   +/-   ##
=======================================
  Coverage   92.70%   92.70%           
=======================================
  Files          55       55           
  Lines        2138     2138           
=======================================
  Hits         1982     1982           
  Misses        156      156

Flag	Coverage Δ
unittests	`92.70% <ø> (ø)`

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dvilelaf

Looks good. Some failing e2e.

Adamantios · 2022-09-29T07:57:49Z

Looks good. Some failing e2e.

Previous run failed because of #1168.

Adamantios added 12 commits September 28, 2022 17:50

refactor: replace history_start with n_observations

c904748

This results in better DevX. It is more intuitive to set the number of observations than the exact date.

refactor: replace start with property and add ts_length

aa74afa

docs: explain the reason that end is Optional

36c3783

docs: add missing validation example

1a0a66b

refactor: utilize new properties introduced in cc1620e1

51feb62

refactor: remove superfluous log message

aaa13c1

refactor: increase max_attempts for amm-net

73e8e03

Closes #1396.

chore: increase timeout

4bd227a

Windows tests occasionally timeout.

test: add tests for the new properties introduced in models

f69e268

cc1620e1b6e11c5e339fdbfe6bc2e65544fa8aac

chore: run generators

554269b

Adamantios added the enhancement New feature or request label Sep 28, 2022

Adamantios requested a review from dvilelaf September 28, 2022 15:21

Adamantios added refactor and removed enhancement New feature or request labels Sep 28, 2022

Adamantios commented Sep 28, 2022

View reviewed changes

Karrenbelt reviewed Sep 28, 2022

View reviewed changes

docs/api/plugins/aea_test_autonomy/fixture_helpers.md Show resolved Hide resolved

Karrenbelt reviewed Sep 28, 2022

View reviewed changes

packages/valory/skills/apy_estimation_abci/tests/test_models.py Outdated Show resolved Hide resolved

Karrenbelt reviewed Sep 28, 2022

View reviewed changes

packages/valory/skills/apy_estimation_abci/tests/test_behaviours.py Show resolved Hide resolved

dvilelaf reviewed Sep 28, 2022

View reviewed changes

refactor: simplify statement

e8fd983

DavidMinarsch changed the title ~~Replace start timestamp with number of observations~~ [0.3.2] Replace start timestamp with number of observations Sep 29, 2022

dvilelaf approved these changes Sep 29, 2022

View reviewed changes

Adamantios requested a review from DavidMinarsch September 29, 2022 07:58

DavidMinarsch approved these changes Sep 29, 2022

View reviewed changes

DavidMinarsch merged commit 6797744 into main Sep 29, 2022

DavidMinarsch deleted the refactor/n-observations branch September 29, 2022 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[0.3.2] Replace start timestamp with number of observations #1402

[0.3.2] Replace start timestamp with number of observations #1402

Adamantios commented Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022

Adamantios Sep 28, 2022

Karrenbelt Sep 28, 2022

dvilelaf Sep 28, 2022

Adamantios Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022

Adamantios Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022

Adamantios Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022

Karrenbelt Sep 28, 2022

Adamantios Sep 28, 2022 •

edited

Loading

Karrenbelt Sep 28, 2022

Karrenbelt Sep 28, 2022

Adamantios Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

dvilelaf left a comment

Adamantios commented Sep 29, 2022

		history_start: ${SERVICE_APY_HISTORY_START:int:null}
		n_observations: ${SERVICE_APY_N_OBSERVATIONS:int:120}

	def __validate_params(self) -> None:
	"""Validate the given parameters."""
	# Eventually, we should probably validate all the parameters. E.g., `ts_length` should be < `end`
	for param_name in ("timeout", "window_size"):
	param_val = self.optimizer_params.get(param_name)
	if param_val is not None and not isinstance(param_val, int):
	raise ValueError(
	f"Optimizer's parameter `{param_name}` can be either of type `int` or `None`. "
	f"{type(param_val)} was given."
	)
	# if the value did not exist in the config, then we set it to the default (None) returned from `.get` method
	self.optimizer_params[param_name] = param_val

		unit_amount_from_sec(self.params.end - self.params.start, self._unit)
		unit_amount_from_sec(self.params.ts_length, self._unit)

	@property
	def ts_length(self) -> int:
	"""The length of the timeseries in seconds."""
	return self.n_observations * self.interval

		# filter out end values that will result in negative `start`
		assume(end >= n_observations * interval)

[0.3.2] Replace start timestamp with number of observations #1402

[0.3.2] Replace start timestamp with number of observations #1402

Conversation

Adamantios commented Sep 28, 2022 • edited Loading

Proposed changes

Fixes

Types of changes

Checklist

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adamantios Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adamantios Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adamantios Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adamantios Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adamantios Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

dvilelaf left a comment

Choose a reason for hiding this comment

Adamantios commented Sep 29, 2022

Adamantios commented Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022 •

edited

Loading

Adamantios Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading