-
Notifications
You must be signed in to change notification settings - Fork 7
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
Introduce neuro config quota show
#1156
Changes from 5 commits
632ad2d
404f5d7
768a8d7
9a4a488
9b7a92b
321d64d
574284b
cbee0c6
d696a91
3f05ebb
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 |
---|---|---|
@@ -0,0 +1,59 @@ | ||
from dataclasses import dataclass | ||
from typing import Any, Dict, Optional | ||
|
||
from yarl import URL | ||
|
||
from neuromation.api.config import _Config | ||
from neuromation.api.core import _Core | ||
from neuromation.api.utils import NoPublicConstructor | ||
|
||
|
||
@dataclass(frozen=True) | ||
class QuotaDetails: | ||
spent_minutes: int | ||
limit_minutes: Optional[int] | ||
|
||
@property | ||
def remain_minutes(self) -> Optional[int]: | ||
if self.limit_minutes is None: | ||
# remain: infinity | ||
return None | ||
if self.limit_minutes > self.spent_minutes: | ||
return self.limit_minutes - self.spent_minutes | ||
return 0 | ||
|
||
|
||
@dataclass(frozen=True) | ||
class QuotaInfo: | ||
name: str | ||
gpu_details: QuotaDetails | ||
cpu_details: QuotaDetails | ||
|
||
|
||
class Quota(metaclass=NoPublicConstructor): | ||
def __init__(self, core: _Core, config: _Config) -> None: | ||
self._core = core | ||
self._config = config | ||
|
||
async def get(self) -> QuotaInfo: | ||
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. How to get quota for another user? 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. do we really need a funcitonality to see other user's quota? cc @mariyadavydova 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 obviously support it by REST API. Otherwise please modify the server as well. 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. Ideally, yes, we need a functionality to see other user's quota (for example, I would like to be able to see the quotas left for our free tier users). However, 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.
Please, don't :) I use this functionality to bump quotas via REST API. 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. @mariyadavydova here is a public API class, not 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. Oh, I'm sorry. Then I think that we should update this class accordingly and keep it as is for a while, as this quota functionality is emerging now and we do not have a clear understanding of how it should be organized. 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.
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. already done |
||
url = URL(f"stats/users/{self._config.auth_token.username}") | ||
async with self._core.request("GET", url) as resp: | ||
res = await resp.json() | ||
return _quota_info_from_api(res) | ||
|
||
|
||
def _quota_info_from_api(payload: Dict[str, Any]) -> QuotaInfo: | ||
total_gpu_str = payload["quota"].get("total_gpu_run_time_minutes") | ||
total_cpu_str = payload["quota"].get("total_non_gpu_run_time_minutes") | ||
|
||
gpu_details = QuotaDetails( | ||
spent_minutes=int(payload["jobs"]["total_gpu_run_time_minutes"]), | ||
limit_minutes=int(total_gpu_str) if total_gpu_str else None, | ||
) | ||
cpu_details = QuotaDetails( | ||
spent_minutes=int(payload["jobs"]["total_non_gpu_run_time_minutes"]), | ||
limit_minutes=int(total_cpu_str) if total_gpu_str else None, | ||
) | ||
return QuotaInfo( | ||
name=payload["name"], gpu_details=gpu_details, cpu_details=cpu_details | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
login_with_token as api_login_with_token, | ||
logout as api_logout, | ||
) | ||
from neuromation.cli.formatters.config import QuotaInfoFormatter | ||
|
||
from .formatters import ConfigFormatter | ||
from .root import Root | ||
|
@@ -28,6 +29,11 @@ def config() -> None: | |
"""Client configuration.""" | ||
|
||
|
||
@group() | ||
def quota() -> None: | ||
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. Don't use nested group, 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. AFAIK, the team decided to use 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 decided to put quota management into the config group, that's it. 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. I think the common user cannot change quota. 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. Also please keep in mind: later, after organizations implementing, we may find that quota better fits somewhere in the organization management structures. Now we don't add top-level 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. OK |
||
"""Quota configuration.""" | ||
|
||
|
||
@command() | ||
@async_cmd() | ||
async def show(root: Root) -> None: | ||
|
@@ -38,6 +44,17 @@ async def show(root: Root) -> None: | |
click.echo(fmt(root)) | ||
|
||
|
||
@command("show") | ||
@async_cmd() | ||
async def quota_show(root: Root) -> None: | ||
""" | ||
Print quota and remaining computation time. | ||
""" | ||
quota = await root.client.quota.get() | ||
fmt = QuotaInfoFormatter() | ||
click.echo(fmt(quota)) | ||
|
||
|
||
@command() | ||
@async_cmd() | ||
async def show_token(root: Root) -> None: | ||
|
@@ -194,3 +211,6 @@ async def docker(root: Root, docker_config: str) -> None: | |
config.add_command(docker) | ||
|
||
config.add_command(logout) | ||
config.add_command(quota) | ||
|
||
quota.add_command(quota_show) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
from typing import Callable | ||
|
||
from aiohttp import web | ||
|
||
from neuromation.api import Client | ||
from neuromation.api.quota import QuotaDetails, QuotaInfo | ||
from tests import _TestServerFactory | ||
|
||
|
||
_MakeClient = Callable[..., Client] | ||
|
||
|
||
async def test_quota_get( | ||
aiohttp_server: _TestServerFactory, make_client: _MakeClient | ||
) -> None: | ||
async def handle_stats(request: web.Request) -> web.StreamResponse: | ||
data = { | ||
"name": request.match_info["name"], | ||
"jobs": { | ||
"total_gpu_run_time_minutes": 101, | ||
"total_non_gpu_run_time_minutes": 102, | ||
}, | ||
"quota": { | ||
"total_gpu_run_time_minutes": 201, | ||
"total_non_gpu_run_time_minutes": 202, | ||
}, | ||
} | ||
return web.json_response(data) | ||
|
||
app = web.Application() | ||
app.router.add_get("/stats/users/{name}", handle_stats) | ||
|
||
srv = await aiohttp_server(app) | ||
|
||
async with make_client(srv.make_url("/")) as client: | ||
quota = await client.quota.get() | ||
assert quota == QuotaInfo( | ||
name=client.username, | ||
gpu_details=QuotaDetails(spent_minutes=101, limit_minutes=201), | ||
cpu_details=QuotaDetails(spent_minutes=102, limit_minutes=202), | ||
) | ||
|
||
|
||
async def test_quota_get_no_quota( | ||
aiohttp_server: _TestServerFactory, make_client: _MakeClient | ||
) -> None: | ||
async def handle_stats(request: web.Request) -> web.StreamResponse: | ||
data = { | ||
"name": request.match_info["name"], | ||
"jobs": { | ||
"total_gpu_run_time_minutes": 101, | ||
"total_non_gpu_run_time_minutes": 102, | ||
}, | ||
"quota": {}, | ||
} | ||
return web.json_response(data) | ||
|
||
app = web.Application() | ||
app.router.add_get("/stats/users/{name}", handle_stats) | ||
|
||
srv = await aiohttp_server(app) | ||
|
||
async with make_client(srv.make_url("/")) as client: | ||
quota = await client.quota.get() | ||
assert quota == QuotaInfo( | ||
name=client.username, | ||
gpu_details=QuotaDetails(spent_minutes=101, limit_minutes=None), | ||
cpu_details=QuotaDetails(spent_minutes=102, limit_minutes=None), | ||
) |
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.
Don't use minutes in API but float seconds.
float("inf") is for infinity, please use this constant instead of
None
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.
for this, we will need to change the server-side API. Note, currently, we get the time in minutes (see neuro-inc/platform-api#901).
Should we change the API?
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.
Not necessary, you can multiply by 60 on the client.
In python there are two time representations: float and datetime.
It is confusing enough right now, adding yet another notation makes everything even worse and less usable: people will always convert minutes into something more convenient.
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.
Why do we need
QuotaDetails
? Why not use justfloat
orOptional[timedelta]
?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.
OK, I will change
..._minutes: int
to..._seconds: float
everywhere. But for me personally it'd be confusing if I seeGPU spent: 00h 01m 00s
, then I run a job for 30 seconds and I still see:GPU spent: 00h 01m 00s
. I think, if we need to show time in seconds, the API protocol should be changed.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.
In the first version of my code, there were just fields
gpu_spent_minutes
,cpu_limit_minutes
, etc. But since the lines forCPU
andGPU
should use the same formatting method_format_quota_details
, I found it useful to introduce an additional abstractionQuotaDetails
. Though, if it's an eyesore for you, I'm OK to get rid of it. Should I? cc @serhiy-storchakaThere 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.
API uses seconds (omit the suffix from field names please).
CLI can stay with minutes, it makes sense.
Like we return file size as bytes in the storage API but show it as gigabytes for large files in CLI.
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.
Flat structure with short field names is better IMHO
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.
Fixed.
fixed to use a flat structure.