Introduce `neuro config quota show` #1156

atemate · 2019-11-08T10:16:16Z

atemate · 2019-11-08T10:19:13Z

GET /stats/users/{name} is currently deployed only on dev: default cluster only

asvetlov

The new public API requires documentation

asvetlov · 2019-11-08T10:37:19Z

neuromation/api/quota.py

+
+@dataclass(frozen=True)
+class QuotaDetails:
+    spent_minutes: int


Don't use minutes in API but float seconds.
float("inf") is for infinity, please use this constant instead of None

Don't use minutes in API but float seconds.

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?

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.

Why do we need QuotaDetails? Why not use just float or Optional[timedelta]?

Not necessary, you can multiply by 60 on the client.

OK, I will change ..._minutes: int to ..._seconds: float everywhere. But for me personally it'd be confusing if I see GPU 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.

Why do we need QuotaDetails? Why not use just float or Optional[timedelta]?

In the first version of my code, there were just fields gpu_spent_minutes, cpu_limit_minutes, etc. But since the lines for CPU and GPU should use the same formatting method _format_quota_details, I found it useful to introduce an additional abstraction QuotaDetails. Though, if it's an eyesore for you, I'm OK to get rid of it. Should I? cc @serhiy-storchaka

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.

Flat structure with short field names is better IMHO

API uses seconds (omit the suffix from field names please). CLI can stay with minutes, it makes sense.

Fixed.

Why do we need QuotaDetails? Why not use just float or Optional[timedelta]?

fixed to use a flat structure.

asvetlov · 2019-11-08T10:40:09Z

neuromation/api/quota.py

+        self._core = core
+        self._config = config
+
+    async def get(self) -> QuotaInfo:


How to get quota for another user?
I can do it if have appropriate permissions, right?

do we really need a funcitonality to see other user's quota? cc @mariyadavydova

We obviously support it by REST API. Otherwise please modify the server as well.

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, config seems to be a bad place for such functionality, as other config commands refer to your personal information (or your server's). Let's file another issue and think where we should put the quota-related functionality (this refers to other discussions in this PR too). This particular issue should solve the immediate problem - "How do I, free tier user, get to know how much quota I have?".

We obviously support it by REST API. Otherwise please modify the server as well.

Please, don't :) I use this functionality to bump quotas via REST API.

@mariyadavydova here is a public API class, not neuro config CLI command group.
API is more generic (and potentially harder to change publishing).

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.

async def get(self, user: Optional[str]=None), the default None is for self.

already done

asvetlov · 2019-11-08T10:40:46Z

neuromation/cli/config.py

@@ -28,6 +29,11 @@ def config() -> None:
    """Client configuration."""


+@group()
+def quota() -> None:


Don't use nested group, neuro config show-quota is just fine.

AFAIK, the team decided to use neuro config quota show in order to extend quota group with additional funcitonality: neuro config quota add, etc. Perhaps, the good solution would be to make the group quota non-nested: neuro quota show

We decided to put quota management into the config group, that's it.
Please don't add redundant nesting.

I think the common user cannot change quota.

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 neuro quota but put a simple command into the config group for this reason.

atemate · 2019-11-08T12:46:39Z

All comments considered, everyone thanks for your review!

atemate · 2019-11-08T12:55:41Z

Currently adding the documentation on API

lgtm-com · 2019-11-08T12:56:41Z

This pull request fixes 5 alerts when merging cbee0c6 into 29585e5 - view on LGTM.com

fixed alerts:

3 for Conflicting attributes in base classes
2 for Module is imported with 'import' and 'import from'

asvetlov · 2019-11-08T12:43:46Z

neuromation/api/quota.py

+    gpu_time_limit: float
+
+    @property
+    def cpu_time_remaining(self) -> float:


Now the property is equal to max(self.cpi_time_spent - self.cpu_time_limit).
Do we need a property at all?

atemate · 2019-11-08T14:20:43Z

Added changelog message
Hide API for quota in order to not expose API that will likely change soon. Now, dataclass _QuotaInfo is protected, and class Client has a temporary protected method _get_quota_info()

asvetlov · 2019-11-08T14:28:31Z

neuromation/api/client.py

@@ -103,3 +106,27 @@ def _get_session_cookie(self) -> Optional["Morsel[str]"]:
            if cookie.key == "NEURO_SESSION":
                return cookie
        return None
+
+    async def _get_quota_info(self, user: Optional[str] = None) -> _QuotaInfo:


Please keep client._quota private API.
There is no need to mess client.py with quota-specific things.

lgtm-com · 2019-11-08T14:31:42Z

This pull request introduces 7 alerts when merging 0634ed8 into 29585e5 - view on LGTM.com

new alerts:

4 for Module is imported with 'import' and 'import from'
3 for Conflicting attributes in base classes

lgtm-com · 2019-11-08T14:47:52Z

This pull request introduces 2 alerts when merging 3f05ebb into 29585e5 - view on LGTM.com

new alerts:

2 for Module is imported with 'import' and 'import from'

Artem Yushkovskiy added 4 commits November 7, 2019 20:00

add quota show

632ad2d

tests

404f5d7

fix, cleanup

768a8d7

cleanup

9a4a488

atemate requested review from asvetlov and mariyadavydova November 8, 2019 10:16

astaff added CLIents platform labels Nov 8, 2019

no todo

9b7a92b

asvetlov requested changes Nov 8, 2019

View reviewed changes

Artem Yushkovskiy added 3 commits November 8, 2019 15:22

review: API get(username), time in seconds

321d64d

review: flat structure

574284b

cleanup

cbee0c6

atemate requested a review from asvetlov November 8, 2019 12:55

asvetlov reviewed Nov 8, 2019

View reviewed changes

add changelog

d696a91

atemate requested a review from asvetlov November 8, 2019 14:21

asvetlov reviewed Nov 8, 2019

View reviewed changes

review on hiding

3f05ebb

atemate force-pushed the ay/quota-show branch from 0634ed8 to 3f05ebb Compare November 8, 2019 14:38

atemate requested a review from asvetlov November 8, 2019 14:38

asvetlov approved these changes Nov 8, 2019

View reviewed changes

atemate merged commit d869cb9 into master Nov 8, 2019

atemate deleted the ay/quota-show branch November 8, 2019 15:11

This was referenced Nov 8, 2019

Introduce neuro config add-quota #1142

Closed

Follow-up to PR #931: Introduce JobPolicyEnforcer class (empty so far) neuro-inc/platform-api#963

Merged

Introduce neuro config quota show #1156

Introduce neuro config quota show #1156

Conversation

atemate commented Nov 8, 2019

atemate commented Nov 8, 2019

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov Nov 8, 2019 • 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

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

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

atemate commented Nov 8, 2019

atemate commented Nov 8, 2019

lgtm-com bot commented Nov 8, 2019

Choose a reason for hiding this comment

atemate commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 8, 2019

lgtm-com bot commented Nov 8, 2019

Introduce `neuro config quota show` #1156

Introduce `neuro config quota show` #1156

asvetlov Nov 8, 2019 •

edited

Loading