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

Better value validation for state parameter of fetch_*_table #1616

Merged
merged 8 commits into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
- Added `ascending` parameter to `fetch_*_table()` methods ([#1602](https://github.com/neptune-ai/neptune-client/pull/1602))
- Added `progress_bar` parameter to `fetch_*_table()` methods ([#1599](https://github.com/neptune-ai/neptune-client/pull/1599))

### Fixes
- Better value validation for `state` parameter of `fetch_*_table()` methods ([#1616](https://github.com/neptune-ai/neptune-client/pull/1616))


## neptune 1.8.6

Expand Down
13 changes: 12 additions & 1 deletion src/neptune/internal/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
__all__ = [
"replace_patch_version",
"verify_type",
"verify_value",
"is_stream",
"is_bool",
"is_int",
Expand Down Expand Up @@ -43,6 +44,7 @@
from glob import glob
from io import IOBase
from typing import (
Any,
Iterable,
List,
Mapping,
Expand Down Expand Up @@ -80,6 +82,11 @@ def verify_type(var_name: str, var, expected_type: Union[type, tuple]):
raise TypeError("{} is a stream, which does not implement read method".format(var_name))


def verify_value(var_name: str, var: Any, expected_values: Iterable[T]) -> None:
if var not in expected_values:
raise ValueError(f"{var_name} must be one of {expected_values} (was `{var}`)")
normandy7 marked this conversation as resolved.
Show resolved Hide resolved


def is_stream(var):
return isinstance(var, IOBase) and hasattr(var, "read")

Expand Down Expand Up @@ -184,11 +191,15 @@ def is_ipython() -> bool:
return False


def as_list(name: str, value: Optional[Union[str, Iterable[str]]]) -> Optional[Iterable[str]]:
def as_list(name: str, value: Optional[Union[str, Iterable[str]]]) -> Iterable[str]:
verify_type(name, value, (type(None), str, Iterable))

if value is None:
return []

if isinstance(value, str):
return [value]

verify_collection_type(name, value, str)

return value
10 changes: 9 additions & 1 deletion src/neptune/metadata_containers/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
Union,
)

from typing_extensions import Literal

from neptune.common.exceptions import NeptuneException
from neptune.envs import CONNECTION_MODE
from neptune.exceptions import InactiveProjectException
Expand All @@ -42,7 +44,9 @@
from neptune.internal.state import ContainerState
from neptune.internal.utils import (
as_list,
verify_collection_type,
verify_type,
verify_value,
)
from neptune.metadata_containers import MetadataContainer
from neptune.metadata_containers.abstract import NeptuneObjectCallback
Expand Down Expand Up @@ -191,7 +195,7 @@ def fetch_runs_table(
self,
*,
id: Optional[Union[str, Iterable[str]]] = None,
state: Optional[Union[str, Iterable[str]]] = None,
state: Optional[Union[Literal["inactive", "active"], Iterable[Literal["inactive", "active"]]]] = None,
owner: Optional[Union[str, Iterable[str]]] = None,
tag: Optional[Union[str, Iterable[str]]] = None,
columns: Optional[Iterable[str]] = None,
Expand Down Expand Up @@ -289,6 +293,10 @@ def fetch_runs_table(
verify_type("sort_by", sort_by, str)
verify_type("ascending", ascending, bool)
verify_type("progress_bar", progress_bar, (type(None), bool, type(ProgressBarCallback)))
verify_collection_type("state", states, str)

for state in states:
verify_value("state", state.lower(), ("inactive", "active"))

if isinstance(limit, int) and limit <= 0:
raise ValueError(f"Parameter 'limit' must be a positive integer or None. Got {limit}.")
Expand Down
4 changes: 1 addition & 3 deletions tests/unit/neptune/new/client/test_run_tables.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
from mock import patch

from neptune import init_project
from neptune.exceptions import NeptuneException
from neptune.internal.backends.neptune_backend_mock import NeptuneBackendMock
from neptune.internal.container_type import ContainerType
from neptune.metadata_containers.metadata_containers_table import (
Expand Down Expand Up @@ -53,6 +52,5 @@ def test_fetch_runs_table_is_case_insensitive(self):
def test_fetch_runs_table_raises_correct_exception_for_incorrect_states(self):
for incorrect_state in ["idle", "running", "some_arbitrary_state"]:
with self.subTest(incorrect_state):
with self.assertRaises(NeptuneException) as context:
with self.assertRaises(ValueError):
self.get_table(state=incorrect_state)
self.assertEquals(f"Can't map RunState to API: {incorrect_state}", str(context.exception))
Loading