Use parametrized os value in workflow #1394

Adamantios · 2022-09-26T05:55:32Z

Proposed changes

Uses the parametrized os value introduced in #1389.

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

Adamantios · 2022-09-26T05:58:55Z

We had forgotten to use the parametrized os' value. Now CI fails because we are using ubuntu-specific installation commands.

We temporarily mark this PR as a draft and will tackle this later.

Adamantios · 2022-09-30T06:34:47Z

.github/workflows/main_workflow.yml

-        sudo apt-get update --fix-missing
-        sudo apt-get autoremove
-        sudo apt-get autoclean


These were not necessary for the job.

The problem is that occassionally an outdated dependency can lead to issues with them not being present.

Though why would we add this in the workflow if this does not affect the CI?

Adamantios · 2022-09-30T06:35:14Z

Pipfile

-docker = "==5.0.2"
+docker = "==6.0.0"


Update docker to avoid conflict with pywin32 on Windows.

Adamantios · 2022-09-30T06:40:55Z

Pipfile

@@ -11,14 +11,13 @@ open-aea-cli-ipfs = "==1.20.0"

 [dev-packages]
 asn1crypto = "==1.4.0"
-atheris = "==2.0.11"


Remove the atheris dependency because it is not supported on Windows.

The options would be to:

Add atheris in the setup.py with a condition to install only if not on Windows. This however would add an unnecessary dependency to our framework.

Replace pipenv with poetry which supports "environment markers" and install dev dependency only if not on Windows. We could do this if we reopen [WIP] Chore/poetry #93.

Remove the dependency and let the dev install it if needed. This does not cause any issues since we already have the following logic in the fuzzy tests:

try: import atheris # type: ignore except (ImportError, ModuleNotFoundError): pytestmark = pytest.mark.skip

The best option, for now, would be (3.).

Adamantios · 2022-09-30T06:41:24Z

README.md

+  > :information_source: Note: we are using [atheris](https://github.com/google/atheris) in order to perform fuzzy testing. 
+  > The dependency is not listed in the `Pipfile` because it is not supported on Windows. 
+  > If you need to run or implement a fuzzy test, please manually install the dependency.
+  > If you are developing on Mac, please follow the extra steps described [here](https://github.com/google/atheris#mac).


Added clarification re the atheris dependency.

Adamantios · 2022-09-30T06:42:16Z

setup.py

-def _get_docker_dependency() -> str:
-    """
-    Get the `docker-py` dependency, according to the underlying platform.
-
-    If on Windows, we need to install the cutting-edge package version of `docker` directly from the GitHub repository;
-    as the PyPI distribution of `docker` causes the following conflict:
-
-        docker 5.0.3 depends on pywin32==227; sys_platform == "win32"
-        open-aea[all] 1.20.0 depends on pywin32==304
-
-    Instead, at commit docker/docker-py@3f0095a, `setup.py` has an extra that forces pywin32>=304.
-    """
-    return (
-        "docker==5.0.3"
-        if platform.system() != "Windows"
-        else "docker @ git+https://github.com/docker/docker-py.git@3f0095a7c1966c521652314e524ff362c24ff58c"
-    )


Not necessary since we updated docker.

Same question as above

We do this in order to avoid a conflict with `pywin32` on Windows.

It is not supported on Windows, and we also have this logic in our fuzzy tests: ```python try: import atheris # type: ignore except (ImportError, ModuleNotFoundError): pytestmark = pytest.mark.skip ``` Therefore, the best option for now would be to manually install `atheris` if needed.

No need to manually install python on Windows anymore (5172f47) since we are using `actions/setup-python@master`.

# Conflicts: # .spelling

codecov · 2022-09-30T08:42:09Z

Codecov Report

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

Coverage data is based on head (38e9505) compared to base (218df30).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1394   +/-   ##
=======================================
  Coverage   92.73%   92.73%           
=======================================
  Files          55       55           
  Lines        2146     2146           
=======================================
  Hits         1990     1990           
  Misses        156      156

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

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.

Adamantios · 2022-09-30T08:50:08Z

Failing test on macos reports:

Unreliable test timings! On an initial run, this test took 4940.30ms, which exceeded the deadline of 2000.00ms, but on a subsequent run it took 6.89 ms, which did not. If you expect this sort of variability in your test timings, consider turning deadlines off for this test by setting deadline=None.

I think this variability is expected @Karrenbelt. Let's increase the deadline for hypothesis.

Failing TestABCIAPYEstimationSingleAgent e2e caused by #1168.

DavidMinarsch · 2022-09-30T08:58:59Z

.github/workflows/main_workflow.yml

-        sudo apt-get update --fix-missing
-        sudo apt-get autoremove
-        sudo apt-get autoclean


The problem is that occassionally an outdated dependency can lead to issues with them not being present.

DavidMinarsch · 2022-09-30T09:00:57Z

plugins/aea-test-autonomy/setup.py

 base_deps = [
    "open-aea[all]>=1.20.0,<2.0.0",
    "pytest==7.0.0",
    "open-aea-ledger-ethereum==1.20.0",
-    _get_docker_dependency(),
+    "docker==6.0.0",


@angrybayblade is that compatible with the rest of the plugins etc?

https://github.com/valory-xyz/open-autonomy/blob/main/setup.py#L65

@Adamantios Please update this as well

Also, it would be nice if we can use a range instead of a strict version specifier

Please update this as well

I have:

open-autonomy/setup.py

Line 61 in 38e9505

"docker==6.0.0",

Also, it would be nice if we can use a range instead of a strict version specifier

We can't:
#1394 (comment)

DavidMinarsch · 2022-09-30T09:01:19Z

setup.py

-def _get_docker_dependency() -> str:
-    """
-    Get the `docker-py` dependency, according to the underlying platform.
-
-    If on Windows, we need to install the cutting-edge package version of `docker` directly from the GitHub repository;
-    as the PyPI distribution of `docker` causes the following conflict:
-
-        docker 5.0.3 depends on pywin32==227; sys_platform == "win32"
-        open-aea[all] 1.20.0 depends on pywin32==304
-
-    Instead, at commit docker/docker-py@3f0095a, `setup.py` has an extra that forces pywin32>=304.
-    """
-    return (
-        "docker==5.0.3"
-        if platform.system() != "Windows"
-        else "docker @ git+https://github.com/docker/docker-py.git@3f0095a7c1966c521652314e524ff362c24ff58c"
-    )


Same question as above

#1394 (comment)

# Conflicts: # autonomy/constants.py # docs/autonomy.md # docs/counter_example.md # docs/guides/quick_start.md # docs/package_list.md # docs/price_oracle_intro.md # docs/simple_abci.md # packages/packages.json # packages/valory/agents/abstract_abci/aea-config.yaml # packages/valory/agents/apy_estimation/aea-config.yaml # packages/valory/agents/counter/aea-config.yaml # packages/valory/agents/hello_world/aea-config.yaml # packages/valory/agents/oracle/aea-config.yaml # packages/valory/agents/register_reset/aea-config.yaml # packages/valory/agents/registration_start_up/aea-config.yaml # packages/valory/agents/simple_abci/aea-config.yaml # packages/valory/agents/test_abci/aea-config.yaml # packages/valory/services/apy_estimation/service.yaml # packages/valory/services/apy_estimation_hardhat/service.yaml # packages/valory/services/counter/service.yaml # packages/valory/services/hello_world/service.yaml # packages/valory/services/oracle_goerli/service.yaml # packages/valory/services/oracle_hardhat/service.yaml # packages/valory/services/oracle_polygon/service.yaml # packages/valory/services/oracle_ropsten/service.yaml # packages/valory/services/oracle_ropsten_cluster/service.yaml # packages/valory/services/register_reset/service.yaml # packages/valory/services/simple_abci/service.yaml # packages/valory/skills/abstract_round_abci/skill.yaml # packages/valory/skills/apy_estimation_abci/skill.yaml # packages/valory/skills/apy_estimation_chained_abci/skill.yaml # packages/valory/skills/hello_world_abci/skill.yaml # packages/valory/skills/liquidity_provision_abci/skill.yaml # packages/valory/skills/liquidity_rebalancing_abci/skill.yaml # packages/valory/skills/oracle_abci/skill.yaml # packages/valory/skills/oracle_deployment_abci/skill.yaml # packages/valory/skills/price_estimation_abci/skill.yaml # packages/valory/skills/register_reset_abci/skill.yaml # packages/valory/skills/registration_abci/skill.yaml # packages/valory/skills/reset_pause_abci/skill.yaml # packages/valory/skills/safe_deployment_abci/skill.yaml # packages/valory/skills/simple_abci/skill.yaml # packages/valory/skills/test_abci/skill.yaml # packages/valory/skills/transaction_settlement_abci/skill.yaml # plugins/aea-test-autonomy/setup.py # setup.py

Adamantios · 2022-09-30T12:38:33Z

packages/valory/connections/abci/tests/test_abci_fuzz.py

-    @settings(deadline=2000, suppress_health_check=[HealthCheck.too_slow])
+    @settings(deadline=5000, suppress_health_check=[HealthCheck.too_slow])


#1394 (comment)

Adamantios · 2022-09-30T12:38:39Z

packages/valory/connections/abci/tests/test_abci_fuzz.py

-    @settings(deadline=2000, suppress_health_check=[HealthCheck.too_slow])
+    @settings(deadline=5000, suppress_health_check=[HealthCheck.too_slow])


#1394 (comment)

chore: use parametrized os value

dcd1724

Adamantios marked this pull request as draft September 26, 2022 10:16

Adamantios force-pushed the chore/parametrize-lock-os branch 5 times, most recently from 620398b to f9dad99 Compare September 29, 2022 13:06

fix: parametrize pipenv's installation based on OS

5172f47

Adamantios force-pushed the chore/parametrize-lock-os branch from f9dad99 to e8c1486 Compare September 30, 2022 06:00

Adamantios commented Sep 30, 2022

View reviewed changes

Adamantios force-pushed the chore/parametrize-lock-os branch from adb5d73 to 648ca96 Compare September 30, 2022 06:54

Adamantios added 5 commits September 30, 2022 10:12

fix: update docker

67d0f90

We do this in order to avoid a conflict with `pywin32` on Windows.

chore: do not specify exact version

27ca643

No need to manually install python on Windows anymore (5172f47) since we are using `actions/setup-python@master`.

docs: add clarification about atheris

719561b

Merge branch 'main' into chore/parametrize-lock-os

35772fc

# Conflicts: # .spelling

Adamantios force-pushed the chore/parametrize-lock-os branch from 648ca96 to 35772fc Compare September 30, 2022 07:13

chore: remove unused import

01b1218

DavidMinarsch reviewed Sep 30, 2022

View reviewed changes

Adamantios added 2 commits September 30, 2022 13:24

test: increase hypothesis deadline

9f11883

#1394 (comment)

Adamantios requested a review from angrybayblade September 30, 2022 12:31

Adamantios commented Sep 30, 2022

View reviewed changes

Adamantios marked this pull request as ready for review September 30, 2022 12:55

angrybayblade approved these changes Sep 30, 2022

View reviewed changes

Adamantios requested a review from DavidMinarsch September 30, 2022 14:08

DavidMinarsch approved these changes Sep 30, 2022

View reviewed changes

DavidMinarsch merged commit 8f71c3f into main Sep 30, 2022

DavidMinarsch deleted the chore/parametrize-lock-os branch October 1, 2022 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parametrized os value in workflow #1394

Use parametrized os value in workflow #1394

Adamantios commented Sep 26, 2022

Adamantios commented Sep 26, 2022 •

edited

Loading

Adamantios Sep 30, 2022

DavidMinarsch Sep 30, 2022

Adamantios Sep 30, 2022

Adamantios Sep 30, 2022

Adamantios Sep 30, 2022

Adamantios Sep 30, 2022

Adamantios Sep 30, 2022

DavidMinarsch Sep 30, 2022

codecov bot commented Sep 30, 2022 •

edited

Loading

Adamantios commented Sep 30, 2022 •

edited

Loading

DavidMinarsch Sep 30, 2022

DavidMinarsch Sep 30, 2022

angrybayblade Sep 30, 2022

angrybayblade Sep 30, 2022

Adamantios Sep 30, 2022 •

edited

Loading

DavidMinarsch Sep 30, 2022

Adamantios Sep 30, 2022

Adamantios Sep 30, 2022

		@settings(deadline=2000, suppress_health_check=[HealthCheck.too_slow])
		@settings(deadline=5000, suppress_health_check=[HealthCheck.too_slow])

Use parametrized os value in workflow #1394

Use parametrized os value in workflow #1394

Conversation

Adamantios commented Sep 26, 2022

Proposed changes

Fixes

Types of changes

Checklist

Further comments

Adamantios commented Sep 26, 2022 • edited Loading

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

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

codecov bot commented Sep 30, 2022 • edited Loading

Codecov Report

Adamantios commented Sep 30, 2022 • edited Loading

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 30, 2022 • edited Loading

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 commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 30, 2022 •

edited

Loading

Adamantios commented Sep 30, 2022 •

edited

Loading

Adamantios Sep 30, 2022 •

edited

Loading