Skip to content
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

Usage reporting tweaks #536

Merged
merged 6 commits into from
Jul 8, 2021
Merged

Conversation

shadeofblue
Copy link
Contributor

No description provided.

@shadeofblue shadeofblue self-assigned this Jul 7, 2021
@shadeofblue shadeofblue requested a review from a team July 7, 2021 10:06
Comment on lines 190 to 191
parser.add_argument("--num-instances", type=int, default=1, help="The number of instances of the service to spawn")
parser.add_argument("--show-usage", action="store_true", help="Show usage and cost of each instance while running.")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ;)

@@ -39,9 +39,10 @@ class SimpleService(Service):
SIMPLE_SERVICE = "/golem/run/simple_service.py"
SIMPLE_SERVICE_CTL = "/golem/run/simulate_observations_ctl.py"

def __init__(self, *args, instance_name, **kwargs):
def __init__(self, *args, instance_name, show_usage: bool = False, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd either specify types for both instance_name and show_usage, or for neither of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, here, done ;)

@shadeofblue shadeofblue changed the title parametrize the display of usage in the simple service Usage reporting tweaks Jul 7, 2021
Copy link
Contributor

@azawlocki azawlocki left a comment

Choose a reason for hiding this comment

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

Very nice :) I have a number of very minor comments (basically just what a linter would report), but I'd like to hear your opinion on putting some more common code in the examples/utils.py module.

examples/blender/blender.py Show resolved Hide resolved
Comment on lines +4 to +5
from dataclasses import dataclass, field
from datetime import timedelta, datetime
Copy link
Contributor

Choose a reason for hiding this comment

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

These should go before import enum

yapapi/ctx.py Outdated
async def get_usage(self):
async def get_raw_usage(self) -> yaa_ActivityUsage:
"""Get the raw usage vector for the activity bound to this work context.
The value comes directly from the low level API and is not interpreted in any way."""
Copy link
Contributor

Choose a reason for hiding this comment

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

For a multi-line docstring, there should be a blank line after the first (summary) line, see https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings.

Also I think the prevailing convention in yapapi docstrings is to put the closing """ on a new line in multi-line docstrings.

yapapi/ctx.py Outdated
Comment on lines 494 to 495
"""Get the state activity bound to this work context.
The value comes directly from the low level API and is not interpreted in any way."""
Copy link
Contributor

Choose a reason for hiding this comment

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

See my remark above about multi-line docstring formatting.

yapapi/utils.py Outdated
@@ -2,6 +2,7 @@
import asyncio
import logging
from typing import AsyncContextManager, Callable, Optional
from datetime import datetime, timezone, tzinfo
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep imports sorted according to alphabetic ordering of module names

@shadeofblue
Copy link
Contributor Author

Very nice :) I have a number of very minor comments (basically just what a linter would report),

let's just add a linter to our test runs? ... maybe as a non-required step? ;)...

@shadeofblue shadeofblue dismissed azawlocki’s stale review July 8, 2021 08:59

please re-review

@azawlocki
Copy link
Contributor

Very nice :) I have a number of very minor comments (basically just what a linter would report),

let's just add a linter to our test runs? ... maybe as a non-required step? ;)...

Let's, we have an issue for that: #334

Copy link
Contributor

@azawlocki azawlocki left a comment

Choose a reason for hiding this comment

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

LGTM!

@shadeofblue shadeofblue merged commit 92694b9 into master Jul 8, 2021
@shadeofblue shadeofblue deleted the blue/parametrize-simple-service-usage branch July 8, 2021 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants