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

fix: Correct Build Scan Endpoint #3785

Merged
merged 18 commits into from May 3, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
04d5f2b
fix: correct build-scan endpoint with workspace and add parameter
ewagner-verta May 2, 2023
48eb7fe
test: update unit tests for endpoint change
ewagner-verta May 2, 2023
65268b5
chore: black format
ewagner-verta May 2, 2023
7b5fbf0
refactor: include workspace when `Build` object is instantiated and d…
ewagner-verta May 2, 2023
c90a7b7
refactor: update `Endpoint` class to also include workspace when inst…
ewagner-verta May 2, 2023
b204f23
refactor: include workspace when `Build` object is instantiated by RMV
ewagner-verta May 2, 2023
83368e7
test: new hypothesis strategy for workspace name
ewagner-verta May 2, 2023
7a50684
test: set a value for the mock registered model version's `registered…
ewagner-verta May 2, 2023
72972cd
test: drop usage of removed `workspace` parameter and update test to …
ewagner-verta May 2, 2023
6c9af65
test: update tests for addition of workspace to init params of `Build…
ewagner-verta May 2, 2023
dba4496
fix: final newline
ewagner-verta May 2, 2023
6b1cd42
fix: add doc string to test strategy with unicode explanation
ewagner-verta May 3, 2023
c7abe4e
fix: remove unused import
ewagner-verta May 3, 2023
b79852e
fix: parameter ordering for consistency
ewagner-verta May 3, 2023
c209d00
fix: parameter ordering for consistency
ewagner-verta May 3, 2023
76bb6fc
fix: parameter ordering for consistency
ewagner-verta May 3, 2023
e330424
fix: parameter ordering for consistency
ewagner-verta May 3, 2023
6e89618
Update client/verta/tests/unit_tests/strategies.py
ewagner-verta May 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/verta/tests/unit_tests/conftest.py
Expand Up @@ -74,5 +74,5 @@ def __repr__(self): # avoid network calls when displaying test results
return MockRegisteredModelVersion(
mock_conn,
mock_config,
_RegistryService.ModelVersion(id=555),
_RegistryService.ModelVersion(id=555, registered_model_id=123)
ewagner-verta marked this conversation as resolved.
Show resolved Hide resolved
)
24 changes: 17 additions & 7 deletions client/verta/tests/unit_tests/deployment/test_build.py
Expand Up @@ -8,7 +8,7 @@
from hypothesis import given, HealthCheck, settings
from responses.matchers import query_param_matcher

from tests.unit_tests.strategies import build_dict
from tests.unit_tests.strategies import build_dict, mock_workspace
from verta._internal_utils import time_utils
from verta.endpoint.build import Build

Expand All @@ -23,10 +23,10 @@ def assert_build_fields(build: Build, build_dict: Dict[str, Any]) -> None:


@settings(suppress_health_check=[HealthCheck.function_scoped_fixture])
@given(build_dict=build_dict())
def test_instantiation(mock_conn, build_dict):
@given(build_dict=build_dict(), workspace=mock_workspace())
def test_instantiation(mock_conn, build_dict, workspace):
"""Verify a Build object can be instantated from a dict."""
build = Build(mock_conn, build_dict)
build = Build(mock_conn, workspace, build_dict)

assert_build_fields(build, build_dict)

Expand Down Expand Up @@ -66,6 +66,8 @@ def test_model_version_list_builds(
build_dicts,
):
"""Verify we can construct Build objects from list_builds()."""
registry_url = f"{mock_conn.scheme}://{mock_conn.socket}/api/v1/registry"
model_version_url = f"{registry_url}/registered_models/{mock_registered_model_version.registered_model_id}"
deployment_url = f"{mock_conn.scheme}://{mock_conn.socket}/api/v1/deployment"
list_builds_url = f"{deployment_url}/builds"

Expand All @@ -80,6 +82,11 @@ def test_model_version_list_builds(
],
json={"builds": build_dicts},
)
rsps.get(
url=model_version_url,
status=200,
json={"workspace_id": "123"},
)
ewagner-verta marked this conversation as resolved.
Show resolved Hide resolved

builds = mock_registered_model_version.list_builds()

Expand All @@ -95,12 +102,15 @@ def test_model_version_list_builds(
assert_build_fields(build, build_dict)


@given(build_dict=build_dict())
def test_build_start_scan_not_external_exception(mock_conn, build_dict):
@given(
build_dict=build_dict(),
workspace=mock_workspace(),
)
def test_build_start_scan_not_external_exception(mock_conn, build_dict, workspace):
"""Test that start_scan() raises the expected error when the `external` parameter is not
included or False. This can be removed when we support internal scans.
"""
build = Build(mock_conn, build_dict)
build = Build(mock_conn, workspace, build_dict)
with pytest.raises(
NotImplementedError,
match=(
Expand Down
22 changes: 15 additions & 7 deletions client/verta/tests/unit_tests/deployment/test_build_scan.py
Expand Up @@ -5,7 +5,7 @@
from hypothesis import given, HealthCheck, settings
import pytest

from tests.unit_tests.strategies import build_dict, build_scan_dict
from tests.unit_tests.strategies import build_dict, build_scan_dict, mock_workspace

from verta._internal_utils import time_utils
from verta.endpoint.build import Build, BuildScan, ScanProgressEnum, ScanResultEnum
Expand Down Expand Up @@ -36,10 +36,14 @@ def test_instantiation(build_scan_dict):


@settings(suppress_health_check=[HealthCheck.function_scoped_fixture])
@given(build_dict=build_dict(), build_scan_dict=build_scan_dict())
def test_get_scan(mock_conn, mocked_responses, build_dict, build_scan_dict):
@given(
build_dict=build_dict(),
build_scan_dict=build_scan_dict(),
workspace=mock_workspace(),
)
def test_get_scan(mock_conn, mocked_responses, build_dict, build_scan_dict, workspace):
"""Verify we can construct a BuildScan object from get_scan()."""
build = Build(mock_conn, build_dict)
build = Build(mock_conn, workspace, build_dict)

deployment_url = f"{mock_conn.scheme}://{mock_conn.socket}/api/v1/deployment"
scan_url = f"{deployment_url}/builds/{build.id}/scan"
Expand All @@ -51,17 +55,21 @@ def test_get_scan(mock_conn, mocked_responses, build_dict, build_scan_dict):

assert_build_scan_fields(build_scan, build_scan_dict)


@settings(suppress_health_check=[HealthCheck.function_scoped_fixture])
@given(
build_dict=build_dict(external_scan=True),
build_scan_dict=build_scan_dict(external_scan=True),
workspace=mock_workspace(),
)
def test_start_external_scan(mock_conn, mocked_responses, build_dict, build_scan_dict):
def test_start_external_scan(
mock_conn, mocked_responses, build_dict, build_scan_dict, workspace
):
"""Verify we can construct a BuildScan object from start_scan(external=True)."""
build = Build(mock_conn, build_dict)
build = Build(mock_conn, workspace, build_dict)

deployment_url = f"{mock_conn.scheme}://{mock_conn.socket}/api/v1/deployment"
scan_url = f"{deployment_url}/builds/{build.id}/scan"
scan_url = f"{deployment_url}/workspace/{workspace}/builds/{build.id}/scan"
ewagner-verta marked this conversation as resolved.
Show resolved Hide resolved

with mocked_responses as rsps:
rsps.post(url=scan_url, status=200, json=build_scan_dict)
Expand Down
16 changes: 16 additions & 0 deletions client/verta/tests/unit_tests/strategies.py
Expand Up @@ -221,3 +221,19 @@ def build_scan_dict(draw, external_scan: Optional[bool] = None) -> Dict[str, Any
d["details"] = draw(st.lists(_build_scan_detail()))

return d


@st.composite
def mock_workspace(draw):
""" Return a valid workspace name.
Unicode categories allowed: Ll (lowercase letter), Lu (Uppercase letters),
Nl (Decimal number), Pd (Dash punctuation). `%` disallowed to prevent
ewagner-verta marked this conversation as resolved.
Show resolved Hide resolved
url encoding issues when testing.
"""
workspace = draw(
st.text(
alphabet=st.characters(whitelist_categories=("Ll", "Lu", "Nd", "Pd")),
min_size=1,
)
)
return workspace
2 changes: 1 addition & 1 deletion client/verta/verta/endpoint/_endpoint.py
Expand Up @@ -462,7 +462,7 @@ def _create_build(self, model_reference):
)

_utils.raise_for_http_error(response)
return Build(self._conn, response.json())
return Build(self._conn, self.workspace, response.json())

def _get_or_create_stage(self, name="production"):
"""Return a stage id for compatibility reasons at the moment."""
Expand Down
19 changes: 10 additions & 9 deletions client/verta/verta/endpoint/build/_build.py
Expand Up @@ -46,8 +46,9 @@ class Build:

_EMPTY_MESSAGE = "no error message available"

def __init__(self, conn, json):
def __init__(self, conn, workspace, json):
self._conn = conn
self._workspace = workspace
self._json = json

def __repr__(self):
Expand All @@ -59,13 +60,11 @@ def _get(cls, conn: _utils.Connection, workspace: str, id: int) -> "Build":
response = _utils.make_request("GET", url, conn)
_utils.raise_for_http_error(response)

return cls(conn, response.json())
return cls(conn, workspace, response.json())

@classmethod
def _list_model_version_builds(
cls,
conn: _utils.Connection,
id: int,
cls, conn: _utils.Connection, workspace: str, id: int
) -> List["Build"]:
"""Returns a model version's builds."""
url = f"{conn.scheme}://{conn.socket}/api/v1/deployment/builds"
Expand All @@ -74,7 +73,8 @@ def _list_model_version_builds(
_utils.raise_for_http_error(response)

return [
cls(conn, build_json) for build_json in response.json().get("builds", [])
cls(conn, workspace, build_json)
for build_json in response.json().get("builds", [])
]

@property
Expand Down Expand Up @@ -132,7 +132,8 @@ def start_scan(self, external: bool) -> _build_scan.BuildScan:
"""
if not external:
raise NotImplementedError(
"internal scans are not yet supported; please use `external=True`"
" parameter"
"internal scans are not yet supported; please use `external=True` parameter"
)
return _build_scan.BuildScan._create(self._conn, self.id, external=external)
return _build_scan.BuildScan._create(
self._conn, self._workspace, self.id, external=external
)
6 changes: 4 additions & 2 deletions client/verta/verta/endpoint/build/_build_scan.py
Expand Up @@ -101,9 +101,11 @@ def _get(cls, conn: _utils.Connection, build_id: int):
return cls(response.json())

@classmethod
def _create(cls, conn: _utils.Connection, build_id: int, external: bool) -> "BuildScan":
def _create(
cls, conn: _utils.Connection, workspace: str, build_id: int, external: bool
) -> "BuildScan":
data = {"scan_external": external}
url = f"{conn.scheme}://{conn.socket}/api/v1/deployment/builds/{build_id}/scan"
url = f"{conn.scheme}://{conn.socket}/api/v1/deployment/workspace/{workspace}/builds/{build_id}/scan"
response = _utils.make_request("POST", url, conn, json=data)
_utils.raise_for_http_error(response)

Expand Down
2 changes: 1 addition & 1 deletion client/verta/verta/registry/entities/_modelversion.py
Expand Up @@ -1728,5 +1728,5 @@ def list_builds(self) -> List[Build]:
))

"""
builds = Build._list_model_version_builds(self._conn, self.id)
builds = Build._list_model_version_builds(self._conn, self.workspace, self.id)
return sorted(builds, key=lambda build: build.date_created, reverse=True)