Unified task runner scripts #1086

approxit · 2023-01-16T13:08:38Z

Bucke up folks, massive diff is comming!

What I've done:

Unified task runner scripts according to other libraries
Bumped mypy version to be able to install other dependencies
Added pyproject-autoflake because for some reason at current setup autoflake was not picking up config from pyproject.toml
Commented out few flake8 errors and few mypy paths, this should be done as separate work
Added __init__.py files into examples and tests to satisfy mypy.
Renamed few directories in examples to satisfy mypy (as they are now python packages).

Changes are mostly related to formatting or small fixes such as specifying exception types in except blocks, changing == to is for None values, etc.

examples/custom_usage_counter/custom_usage_counter.py

shadeofblue · 2023-02-07T06:37:17Z

hmm, given that we don't treat the examples as modules but rather self-contained scripts, I'm unsure why we'd need __init__ files in there... ?

shadeofblue · 2023-02-07T06:38:35Z

examples/http_proxy/http_proxy.py

@@ -3,10 +3,10 @@
 a simple http proxy example


what is the purpose of this name change?

shadeofblue · 2023-02-07T06:40:01Z

examples/low_level_api/list-offers.py

@@ -1,10 +1,10 @@
 #!/usr/bin/env python3


same here... these are not modules... the name changes here are neither required nor in any way beneficial... and we risk breaking any links pointing to those examples from any external locations... please, let's not rename them unless we have a very good reason

shadeofblue · 2023-02-07T06:41:20Z

tests/drone/drone.py

-from yapapi.log import enable_default_logger
-from yapapi.payload import vm
-from yapapi.rest.activity import BatchTimeoutError
+from yapapi import windows_event_loop_fix  # noqa: E402


we can probably remove windows_event_loop_fix once we drop support for Python < 3.8

shadeofblue · 2023-02-07T06:44:02Z

yapapi/contrib/service/http_proxy.py

@@ -262,14 +258,15 @@ async def _request_handler(self, request: web.Request) -> web.Response:
        return await instance.handle_request(request)

    async def run(self):
+        """Run a local HTTP server.


well, it's not just any HTTP server...

Run a local HTTP proxy server.

shadeofblue · 2023-02-07T06:44:37Z

yapapi/contrib/service/socket_proxy.py

@@ -199,14 +199,14 @@ def __repr__(self):

    @property
    def app_key(self):
-        """The application key used to authorize access to `yagna`'s REST API."""
+        """Return the application key used to authorize access to `yagna`'s REST API."""


hmm, it's not a function ... it's a property... I believe it should be a noun-like not an action-like...

shadeofblue · 2023-02-07T06:44:47Z

yapapi/contrib/service/socket_proxy.py

        return (
            self.service.cluster.service_runner._job.engine._api_config.app_key  # type: ignore[union-attr]  # noqa
        )

    @property
    def instance_ws(self):
-        """The websocket URI for the specific remote port of the Service."""
+        """Return the websocket URI for the specific remote port of the Service."""


same here...

shadeofblue · 2023-02-07T06:47:46Z

yapapi/network.py

@@ -181,39 +172,39 @@ async def __aexit__(self, *exc_info) -> None:

    @property
    def owner_ip(self) -> str:
-        """The IP address of the requestor node within the network."""
+        """Return the IP address of the requestor node within the network."""


again, these are properties, not methods... their docstrings should reflect that...

shadeofblue · 2023-02-07T06:50:13Z

yapapi/rest/configuration.py

@@ -44,27 +42,27 @@ def app_key(self) -> str:

    @property
    def market_url(self) -> str:
-        """The URL of the Market REST API"""
+        """Return URL of the Market REST API."""


and again, in case of properties, I believe the docstrings should be formed just as they would be for attributes rather than functions...

shadeofblue

Please revert the directory name changes in the examples part... unless there's a critical reason to do that

Otherwise, I strongly believe the properties' docstrings should be phrased just as they would be in case of regular attributes rather than functions -> especially that once you receive them, they behave so -> you cannot call them for them to perform their function, you get what the docstring says you'd get, so, e.g.

approxit · 2023-02-09T15:55:20Z

Regarding __init__.py in examples and tests, I've commented at the very begining that it was added to satisfy mypy, because it encounter errors about loading files without __init__.py. I could reconfigure mypy to check specific list of directories, instead of whole codebase.

Regarding property docs - change was made to satisfy pydocstyle - should we fight with this?

examples/simple-service-poc/simple_service/simple_service.py

tests/test_network.py

yapapi/contrib/service/http_proxy.py

shadeofblue

okay, let's say it looks good...

I'd still strongly prefer the properties' docs to be specified using noun-like constructs and not action-like ones but I don't feel this is worth arguing about...

Co-authored-by: shadeofblue <blue@wave460.net>

This was referenced Jan 16, 2023

Add missing docstrings #1087

Open

Fix mypy errors in excluded paths #1088

Open

approxit marked this pull request as ready for review January 20, 2023 13:22

approxit requested review from a team, lucekdudek and shadeofblue January 20, 2023 13:22

approxit assigned shadeofblue and lucekdudek Jan 20, 2023

approxit mentioned this pull request Jan 20, 2023

Use poetry 1.2+ dependency notation for deps variants in pyproject.toml #1089

Open

3 tasks

approxit self-assigned this Jan 23, 2023

lucekdudek reviewed Feb 1, 2023

View reviewed changes

examples/custom_usage_counter/custom_usage_counter.py Outdated Show resolved Hide resolved

lucekdudek approved these changes Feb 1, 2023

View reviewed changes

approxit force-pushed the unify-tasks-across-repos branch from 40e3009 to 693fc25 Compare February 2, 2023 14:25

shadeofblue reviewed Feb 7, 2023

View reviewed changes

shadeofblue requested changes Feb 7, 2023

View reviewed changes

lucekdudek removed their assignment Feb 23, 2023

approxit mentioned this pull request Feb 23, 2023

Remove usage of shebang in examples #1104

Open

approxit force-pushed the unify-tasks-across-repos branch from 8196772 to 40c43e8 Compare March 3, 2023 15:40

approxit mentioned this pull request Mar 3, 2023

Exceptions in Probe.run_command_on_host context are shadowing exceptions from command on host golemfactory/goth#611

Open

approxit requested a review from shadeofblue March 3, 2023 17:53

shadeofblue reviewed Mar 7, 2023

View reviewed changes

examples/simple-service-poc/simple_service/simple_service.py Outdated Show resolved Hide resolved

shadeofblue reviewed Mar 7, 2023

View reviewed changes

tests/test_network.py Show resolved Hide resolved

shadeofblue reviewed Mar 7, 2023

View reviewed changes

yapapi/contrib/service/http_proxy.py Outdated Show resolved Hide resolved

shadeofblue approved these changes Mar 7, 2023

View reviewed changes

approxit and others added 20 commits March 7, 2023 11:55

Unified task runner scripts

b5f3b3e

most of flake8 errors

fadcdd9

mypy partially done

34588bd

fixed workflows

4e7ea6a

fixed ut

188a330

fixed ut again

2f84fe7

fixed sphinx

f9e3428

fixed mypy

264d514

fixed mypy and goth

f2f931c

updated README.md

e815b6c

add log file path to the power_outage test

7e4026c

format fix

f599243

reverted examples renames

fdc137d

fixed utils import in examples

2e1b4d3

example fix

187f991

add logfile, re-add +x chmod

61c8c8e

chmod +x

8db87da

commented out bad goth test

d5b6b0d

Fix test_run_simple_service wrong module name

9733d36

Apply suggestions from code review

4b68e01

Co-authored-by: shadeofblue <blue@wave460.net>

approxit force-pushed the unify-tasks-across-repos branch from 8d7661a to 4b68e01 Compare March 7, 2023 10:55

approxit merged commit ae04684 into master Mar 7, 2023

approxit deleted the unify-tasks-across-repos branch March 7, 2023 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified task runner scripts #1086

Unified task runner scripts #1086

approxit commented Jan 16, 2023 •

edited

Loading

shadeofblue commented Feb 7, 2023

shadeofblue Feb 7, 2023

shadeofblue Feb 7, 2023

shadeofblue Feb 7, 2023

shadeofblue Feb 7, 2023

approxit Feb 10, 2023

shadeofblue Feb 7, 2023

shadeofblue Feb 7, 2023

shadeofblue Feb 7, 2023

shadeofblue Feb 7, 2023

shadeofblue left a comment

approxit commented Feb 9, 2023

shadeofblue left a comment

Unified task runner scripts #1086

Unified task runner scripts #1086

Conversation

approxit commented Jan 16, 2023 • edited Loading

shadeofblue commented Feb 7, 2023

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

Choose a reason for hiding this comment

shadeofblue left a comment

Choose a reason for hiding this comment

approxit commented Feb 9, 2023

shadeofblue left a comment

Choose a reason for hiding this comment

approxit commented Jan 16, 2023 •

edited

Loading