-
Notifications
You must be signed in to change notification settings - Fork 23
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.
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
[0.3.2] Replace start timestamp with number of observations #1402
Changes from 12 commits
c904748
aa74afa
36c3783
1a0a66b
51feb62
aaa13c1
73e8e03
4bd227a
c0be2c0
247e5f7
f69e268
554269b
e8fd983
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7,7 +7,7 @@ license: Apache-2.0 | |||||||||||||||||||||||||
fingerprint: | ||||||||||||||||||||||||||
README.md: bafybeibhbkelnxxvsln677imq65vgbglmlhyxtax4iqtzempjiwcoef3gq | ||||||||||||||||||||||||||
fingerprint_ignore_patterns: [] | ||||||||||||||||||||||||||
agent: valory/apy_estimation:0.1.0:bafybeidt3vndfphaezdksiwcvjwc4tocjn5g5zyl4e4wagdyy7vbmzr6ru | ||||||||||||||||||||||||||
agent: valory/apy_estimation:0.1.0:bafybeidvbcjss3lkw24if4gzi33nsixcxglcluahsp7bzh7omeqhzjdb6a | ||||||||||||||||||||||||||
number_of_agents: 4 | ||||||||||||||||||||||||||
--- | ||||||||||||||||||||||||||
public_id: valory/apy_estimation_abci:0.1.0 | ||||||||||||||||||||||||||
|
@@ -22,7 +22,7 @@ models: | |||||||||||||||||||||||||
spooky_subgraph: | ||||||||||||||||||||||||||
- '0x2a651563c9d3af67ae0388a5c8f89b867038089e' | ||||||||||||||||||||||||||
- '0x2b4c76d0dc16be1c31d4c1dc53bf9b45987fc75c' | ||||||||||||||||||||||||||
history_start: ${SERVICE_APY_HISTORY_START:int:null} | ||||||||||||||||||||||||||
n_observations: ${SERVICE_APY_N_OBSERVATIONS:int:120} | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. There probably is some range that should be allowed, not up to infinity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In aea-config.yaml n_observations is 10 while here it is 120. Is that right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Have you heard of pydantic? Using it might produce some cool results. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, we need the service to run with proper values and the CI to run fast. |
||||||||||||||||||||||||||
history_end: null | ||||||||||||||||||||||||||
ipfs_domain_name: /dns/registry.autonolas.tech/tcp/443/https | ||||||||||||||||||||||||||
server_api: | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -306,7 +306,7 @@ def setup(self) -> None: | |||||||||
|
||||||||||
self._unit = sec_to_unit(self.params.interval) | ||||||||||
self._target_per_pool = int( | ||||||||||
unit_amount_from_sec(self.params.end - self.params.start, self._unit) | ||||||||||
unit_amount_from_sec(self.params.ts_length, self._unit) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now have a property for this: open-autonomy/packages/valory/skills/apy_estimation_abci/models.py Lines 204 to 207 in 554269b
|
||||||||||
) | ||||||||||
n_ids = sum( | ||||||||||
(len(dex_pair_ids) for dex_pair_ids in self.params.pair_ids.values()) | ||||||||||
|
@@ -361,14 +361,15 @@ def _check_given_pairs(self) -> Generator[None, None, None]: | |||||||||
|
||||||||||
def _reset_timestamps_iterator(self) -> None: | ||||||||||
"""Reset the timestamps iterator.""" | ||||||||||
# end is set in the `setup` method and therefore cannot be `None` at this point | ||||||||||
# `start` and `end` are set in the `setup` method and therefore cannot be `None` at this point | ||||||||||
start = cast(int, self.params.start) | ||||||||||
end = cast(int, self.params.end) | ||||||||||
|
||||||||||
if self.batch: | ||||||||||
self._progress.timestamps_iterator = iter((end,)) | ||||||||||
else: | ||||||||||
self._progress.timestamps_iterator = gen_unix_timestamps( | ||||||||||
self.params.start, self.params.interval, end | ||||||||||
start, self.params.interval, end | ||||||||||
) | ||||||||||
|
||||||||||
def _set_current_progress(self) -> None: | ||||||||||
|
@@ -1402,7 +1403,6 @@ def async_act(self) -> Generator: | |||||||||
self.context.logger.info( | ||||||||||
"Estimates have been received:\n" f"{estimates.to_string()}" | ||||||||||
) | ||||||||||
self.context.logger.info("Estimates have been received.") | ||||||||||
|
||||||||||
Comment on lines
1403
to
1406
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed because it was duplicate. |
||||||||||
# Send the file to IPFS and get its hash. | ||||||||||
self._estimations_hash = self.send_to_ipfs( | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,9 +176,11 @@ class APYParams(BaseParams): # pylint: disable=too-many-instance-attributes | |
|
||
def __init__(self, *args: Any, **kwargs: Any) -> None: | ||
"""Initialize the parameters object.""" | ||
self.start: int = self._ensure("history_start", kwargs) | ||
# end can be `None`; this means that the current time will be used | ||
# It is set in the behaviour using the last synced timestamp among the agents | ||
self.end: Optional[int] = kwargs.pop("history_end", None) | ||
self.interval: int = self._ensure("history_interval_in_unix", kwargs) | ||
self.n_observations: int = self._ensure("n_observations", kwargs) | ||
self.optimizer_params: Dict[ | ||
str, Union[None, bool, int, float, str] | ||
] = self._ensure("optimizer", kwargs) | ||
|
@@ -192,9 +194,21 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: | |
|
||
self.__validate_params() | ||
|
||
@property | ||
def start(self) -> Optional[int]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
"""The start timestamp of the timeseries.""" | ||
if self.end is None: | ||
return None | ||
return self.end - self.ts_length | ||
|
||
@property | ||
def ts_length(self) -> int: | ||
"""The length of the timeseries in seconds.""" | ||
return self.n_observations * self.interval | ||
|
||
def __validate_params(self) -> None: | ||
"""Validate the given parameters.""" | ||
# Eventually, we should probably validate all the 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): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Increased timeout as I have noticed windows tests timing out lately and I also added two new
hypothesis
tests.