Improve parsing --jobs and --credits options (GH-2458)

neuro-inc · Dec 6, 2021 · dde0176 · dde0176
1 parent dc96afc
commit dde0176
Show file tree

Hide file tree

Showing 11 changed files with 294 additions and 53 deletions.
diff --git a/CHANGELOG.D/2453.feature b/CHANGELOG.D/2453.feature
@@ -0,0 +1 @@
+Option `-j`/`--jobs` in `neuro admin set-user-quota` is now required. Pass `unlimited` for setting no limit. Negative values are now rejected.
diff --git a/CHANGELOG.D/2454.feature b/CHANGELOG.D/2454.feature
@@ -0,0 +1 @@
+Option `-c`/`--credits` in `neuro admin set-user-credits` and `neuro admin add-user-credits` is now required. Pass "unlimited" in `neuro admin set-user-credits` for setting no limit. Values "infinity", "nan" etc are now rejected.
diff --git a/CLI.md b/CLI.md
@@ -350,8 +350,8 @@ neuro admin add-cluster-user [OPTIONS] CLUSTER_NAME USER_NAME [ROLE]
 Name | Description|
 |----|------------|
 |_--help_|Show this message and exit.|
-|_\-c, --credits AMOUNT_|Credits amount to set|
-|_\-j, --jobs AMOUNT_|Maximum running jobs quota|
+|_\-c, --credits AMOUNT_|Credits amount to set \(`unlimited' stands for no limit)  \[default: unlimited]|
+|_\-j, --jobs AMOUNT_|Maximum running jobs quota \(`unlimited' stands for no limit)  \[default: unlimited]|
 
 
 
@@ -409,7 +409,7 @@ neuro admin set-user-quota [OPTIONS] CLUSTER_NAME USER_NAME
 Name | Description|
 |----|------------|
 |_--help_|Show this message and exit.|
-|_\-j, --jobs AMOUNT_|Maximum running jobs quota|
+|_\-j, --jobs AMOUNT_|Maximum running jobs quota \(`unlimited' stands for no limit)  \[required]|
 
 
 
@@ -429,7 +429,7 @@ neuro admin set-user-credits [OPTIONS] CLUSTER_NAME USER_NAME
 Name | Description|
 |----|------------|
 |_--help_|Show this message and exit.|
-|_\-c, --credits AMOUNT_|Credits amount to set|
+|_\-c, --credits AMOUNT_|Credits amount to set \(`unlimited' stands for no limit)  \[required]|
 
 
 
@@ -449,7 +449,7 @@ neuro admin add-user-credits [OPTIONS] CLUSTER_NAME USER_NAME
 Name | Description|
 |----|------------|
 |_--help_|Show this message and exit.|
-|_\-c, --credits AMOUNT_|Credits amount to add|
+|_\-c, --credits AMOUNT_|Credits amount to add  \[required]|
 
 
 

diff --git a/neuro-cli/docs/admin.md b/neuro-cli/docs/admin.md
@@ -161,8 +161,8 @@ roles: admin, manager or user.
 | Name | Description |
 | :--- | :--- |
 | _--help_ | Show this message and exit. |
-| _-c, --credits AMOUNT_ | Credits amount to set |
-| _-j, --jobs AMOUNT_ | Maximum running jobs quota |
+| _-c, --credits AMOUNT_ | Credits amount to set \(`unlimited' stands for no limit\)  _\[default: unlimited\]_ |
+| _-j, --jobs AMOUNT_ | Maximum running jobs quota \(`unlimited' stands for no limit\)  _\[default: unlimited\]_ |
 
 
 
@@ -226,7 +226,7 @@ Set user quota to given values
 | Name | Description |
 | :--- | :--- |
 | _--help_ | Show this message and exit. |
-| _-j, --jobs AMOUNT_ | Maximum running jobs quota |
+| _-j, --jobs AMOUNT_ | Maximum running jobs quota \(`unlimited' stands for no limit\)  _\[required\]_ |
 
 
 
@@ -248,7 +248,7 @@ Set user credits to given value
 | Name | Description |
 | :--- | :--- |
 | _--help_ | Show this message and exit. |
-| _-c, --credits AMOUNT_ | Credits amount to set |
+| _-c, --credits AMOUNT_ | Credits amount to set \(`unlimited' stands for no limit\)  _\[required\]_ |
 
 
 
@@ -270,7 +270,7 @@ Add given values to user quota
 | Name | Description |
 | :--- | :--- |
 | _--help_ | Show this message and exit. |
-| _-c, --credits AMOUNT_ | Credits amount to add |
+| _-c, --credits AMOUNT_ | Credits amount to add  _\[required\]_ |
 
 
 

diff --git a/neuro-cli/src/neuro_cli/admin.py b/neuro-cli/src/neuro_cli/admin.py
@@ -5,7 +5,7 @@
 import os
 import pathlib
 from dataclasses import replace
-from decimal import Decimal
+from decimal import Decimal, InvalidOperation
 from typing import IO, Any, Mapping, Optional
 
 import click
@@ -26,6 +26,8 @@
 
 log = logging.getLogger(__name__)
 
+UNLIMITED = "unlimited"
+
 
 @group()
 def admin() -> None:
@@ -384,22 +386,26 @@ async def get_cluster_users(root: Root, cluster_name: Optional[str]) -> None:
     "--credits",
     metavar="AMOUNT",
     type=str,
-    help="Credits amount to set",
+    default=UNLIMITED,
+    show_default=True,
+    help="Credits amount to set (`unlimited' stands for no limit)",
 )
 @option(
     "-j",
     "--jobs",
     metavar="AMOUNT",
-    type=int,
-    help="Maximum running jobs quota",
+    type=str,
+    default=UNLIMITED,
+    show_default=True,
+    help="Maximum running jobs quota (`unlimited' stands for no limit)",
 )
 async def add_cluster_user(
     root: Root,
     cluster_name: str,
     user_name: str,
     role: str,
-    credits: Optional[str],
-    jobs: Optional[int],
+    credits: str,
+    jobs: str,
 ) -> None:
     """
     Add user access to specified cluster.
@@ -411,7 +417,7 @@ async def add_cluster_user(
         user_name,
         _ClusterUserRoleType(role),
         balance=_Balance(credits=_parse_credits_value(credits)),
-        quota=_Quota(total_running_jobs=jobs),
+        quota=_Quota(total_running_jobs=_parse_jobs_value(jobs)),
     )
     if not root.quiet:
         root.print(
@@ -422,13 +428,32 @@ async def add_cluster_user(
         )
 
 
-def _parse_credits_value(value: Optional[str]) -> Optional[Decimal]:
-    if value is None:
+def _parse_finite_decimal(value: str) -> Decimal:
+    try:
+        result = Decimal(value)
+        if result.is_finite():
+            return result
+    except (ValueError, LookupError, InvalidOperation):
+        pass
+    raise click.BadParameter(f"{value} is not valid decimal number")
+
+
+def _parse_credits_value(value: str) -> Optional[Decimal]:
+    if value == UNLIMITED:
+        return None
+    return _parse_finite_decimal(value)
+
+
+def _parse_jobs_value(value: str) -> Optional[int]:
+    if value == UNLIMITED:
         return None
     try:
-        return Decimal(value)
-    except (ValueError, LookupError):
-        raise click.BadParameter(f"{value} is not valid decimal number")
+        result = int(value, 10)
+        if result >= 0:
+            return result
+    except ValueError:
+        pass
+    raise click.BadParameter("jobs quota should be non-negative integer")
 
 
 @command()
@@ -480,22 +505,23 @@ async def get_user_quota(
     "-j",
     "--jobs",
     metavar="AMOUNT",
-    type=int,
-    help="Maximum running jobs quota",
+    type=str,
+    required=True,
+    help="Maximum running jobs quota (`unlimited' stands for no limit)",
 )
 async def set_user_quota(
     root: Root,
     cluster_name: str,
     user_name: str,
-    jobs: Optional[int],
+    jobs: str,
 ) -> None:
     """
     Set user quota to given values
     """
     user_with_quota = await root.client._admin.update_cluster_user_quota(
         cluster_name=cluster_name,
         user_name=user_name,
-        quota=_Quota(total_running_jobs=jobs),
+        quota=_Quota(total_running_jobs=_parse_jobs_value(jobs)),
     )
     fmt = AdminQuotaFormatter()
     root.print(
@@ -514,13 +540,14 @@ async def set_user_quota(
     "--credits",
     metavar="AMOUNT",
     type=str,
-    help="Credits amount to set",
+    required=True,
+    help="Credits amount to set (`unlimited' stands for no limit)",
 )
 async def set_user_credits(
     root: Root,
     cluster_name: str,
     user_name: str,
-    credits: Optional[str],
+    credits: str,
 ) -> None:
     """
     Set user credits to given value
@@ -548,6 +575,7 @@ async def set_user_credits(
     "--credits",
     metavar="AMOUNT",
     type=str,
+    required=True,
     help="Credits amount to add",
 )
 async def add_user_credits(
@@ -559,8 +587,7 @@ async def add_user_credits(
     """
     Add given values to user quota
     """
-    additional_credits = _parse_credits_value(credits)
-    assert additional_credits
+    additional_credits = _parse_finite_decimal(credits)
     user_with_quota = await root.client._admin.update_cluster_user_balance_by_delta(
         cluster_name,
         user_name,
@@ -688,7 +715,7 @@ async def add_resource_preset(
     if preset_name in presets:
         raise ValueError(f"Preset '{preset_name}' already exists")
     presets[preset_name] = Preset(
-        credits_per_hour=Decimal(credits_per_hour),
+        credits_per_hour=_parse_finite_decimal(credits_per_hour),
         cpu=cpu,
         memory_mb=memory,
         gpu=gpu,
@@ -781,7 +808,7 @@ async def update_resource_preset(
         raise ValueError(f"Preset '{preset_name}' does not exists")
 
     kwargs = {
-        "credits_per_hour": Decimal(credits_per_hour)
+        "credits_per_hour": _parse_finite_decimal(credits_per_hour)
         if credits_per_hour is not None
         else None,
         "cpu": cpu,

diff --git a/neuro-cli/src/neuro_cli/formatters/config.py b/neuro-cli/src/neuro_cli/formatters/config.py
@@ -144,7 +144,7 @@ def __call__(self, aliases: Iterable[click.Command]) -> Table:
         return table
 
 
-_QUOTA_NOT_SET = "infinity"
+_QUOTA_NOT_SET = "unlimited"
 
 
 def format_quota_details(quota: Optional[Union[int, Decimal]]) -> str:

diff --git a/neuro-cli/tests/unit/formatters/ascii/TestAdminQuotaFormatter.test_output_no_quota_0.ref b/neuro-cli/tests/unit/formatters/ascii/TestAdminQuotaFormatter.test_output_no_quota_0.ref
@@ -1 +1 @@
-Jobs: infinity
+Jobs: unlimited
diff --git a/neuro-cli/tests/unit/formatters/ascii/TestBalanceFormatter.test_output_no_quota_0.ref b/neuro-cli/tests/unit/formatters/ascii/TestBalanceFormatter.test_output_no_quota_0.ref
@@ -1,2 +1,2 @@
-Credits: infinity
+Credits: unlimited
 Spend credits: 0.00
diff --git a/neuro-cli/tests/unit/formatters/ascii/TestClusterUserFormatter.test_cluster_list_0.ref b/neuro-cli/tests/unit/formatters/ascii/TestClusterUserFormatter.test_cluster_list_0.ref
@@ -1,8 +1,8 @@
-╷         ╷                    ╷             ╷                                  ╷          ╷               ╷           
-  Name   │ Role    │ Email              │ Full name   │ Registered                       │ Credits  │ Spent credits │ Max jobs  
-╺━━━━━━━━┿━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━━┿━━━━━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┿━━━━━━━━━━┿━━━━━━━━━━━━━━━┿━━━━━━━━━━╸
-  alex   │ user    │ [email protected]   │             │                                  │ 100.00   │ 20.00         │ 2         
-  andrew │ manager │ [email protected] │ andrew      │ 2017-03-04T12:28:59.759433+00:00 │ 100.00   │ 0.00          │ infinity  
-  denis  │ admin   │ [email protected]  │ denis admin │ 2017-03-04T12:28:59.759433+00:00 │ infinity │ 0.00          │ infinity  
-  ivan   │ user    │ [email protected]   │ user        │ 2017-03-04T12:28:59.759433+00:00 │ infinity │ 0.00          │ 1         
-         ╵         ╵                    ╵             ╵                                  ╵          ╵               ╵
+╷         ╷                    ╷             ╷                                  ╷           ╷               ╷            
+  Name   │ Role    │ Email              │ Full name   │ Registered                       │ Credits   │ Spent credits │ Max jobs   
+╺━━━━━━━━┿━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━━┿━━━━━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┿━━━━━━━━━━━┿━━━━━━━━━━━━━━━┿━━━━━━━━━━━╸
+  alex   │ user    │ [email protected]   │             │                                  │ 100.00    │ 20.00         │ 2          
+  andrew │ manager │ [email protected] │ andrew      │ 2017-03-04T12:28:59.759433+00:00 │ 100.00    │ 0.00          │ unlimited  
+  denis  │ admin   │ [email protected]  │ denis admin │ 2017-03-04T12:28:59.759433+00:00 │ unlimited │ 0.00          │ unlimited  
+  ivan   │ user    │ [email protected]   │ user        │ 2017-03-04T12:28:59.759433+00:00 │ unlimited │ 0.00          │ 1          
+         ╵         ╵                    ╵             ╵                                  ╵           ╵               ╵
diff --git a/neuro-cli/tests/unit/formatters/test_config_formatters.py b/neuro-cli/tests/unit/formatters/test_config_formatters.py
@@ -25,7 +25,7 @@
 @pytest.mark.parametrize(
     "quota,expected",
     [
-        pytest.param(None, "infinity", id="None->infinity"),
+        pytest.param(None, "unlimited", id="None->infinity"),
         pytest.param(0, "0", id="zero"),
         pytest.param(10, "10", id="int"),
         pytest.param(Decimal("1.23456"), "1.23", id="decimal"),
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Option `-j`/`--jobs` in `neuro admin set-user-quota` is now required. Pass `unlimited` for setting no limit. Negative values are now rejected.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Option `-c`/`--credits` in `neuro admin set-user-credits` and `neuro admin add-user-credits` is now required. Pass "unlimited" in `neuro admin set-user-credits` for setting no limit. Values "infinity", "nan" etc are now rejected.