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

[Model monitoring] Init - adding API endpoints #625

Merged
merged 28 commits into from
Jan 11, 2021

Conversation

Michaelliv
Copy link
Contributor

@Michaelliv Michaelliv commented Dec 29, 2020

This PR introduces a restful API that enables querying model endpoint monitoring (v2_mode_serving) related statistics and collected data.

The exposed endpoints are:

  1. create_endpoint - POST /projects/{project}/model-endpoints
  2. delete_endpoint - DELETE /projects/{project}/model-endpoints/{endpoint_id}
  3. list_endpoints - GET /projects/{project}/model-endpoints
  4. get_endpoint - GET /projects/{project}/model-endpoints/{endpoint_id}
  5. get_endpoint_metrics - GET /projects/{project}/model-endpoints/{endpoint_id}/metrics

The tests included in this PR, require the V3IO_API, V3IO_ACCESS_KEY and V3IO_FRAMESD environment parameters to be present at execution.

@Michaelliv Michaelliv changed the title WIP: Model monitoring Model monitoring Dec 31, 2020
@Hedingber Hedingber changed the title Model monitoring [Model monitoring] Init - adding API endpoints Jan 4, 2021
Added metrics query param list_endpoints.
Removed delete_endpoint and create_endpoint.
Implemented clear method.
Updated tests.
Copy link
Member

@theSaarco theSaarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, with good testing coverage. I do have some concerns (see in comments) about project scoping and various semantic issues with how label queries are formed etc.
Also, the main concern I have is about processing time for performing these APIs, and whether they should perhaps be delegated to a different thread to not cause issues with the REST API handling of MLRun in general. After all, some APIs here perform IO operations as well as data aggregations, and these may be heavy, depending on the amount of data being processed.

mlrun/api/api/endpoints/model_endpoints.py Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Show resolved Hide resolved
mlrun/api/schemas/__init__.py Outdated Show resolved Hide resolved
mlrun/api/schemas/model_endpoints.py Outdated Show resolved Hide resolved
tests/api/api/test_model_endpoints.py Outdated Show resolved Hide resolved
tests/api/api/test_model_endpoints.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Michaelliv Looks really good, minor comments, please update the PR description
@theSaarco note that the endpoints are defined with def (and not async def) FastAPI automatically run those in an awaited threadpool so we're good (see here for details)

mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
mlrun/api/schemas/object.py Outdated Show resolved Hide resolved
mlrun/config.py Outdated Show resolved Hide resolved
mlrun/utils/v3io_clients.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some left overs (see unresolved comments):

  1. ModelEndpointMetaData -> ModelEndpointMetadata
  2. staticmethod -> classmethod
  3. You forget to actually return Response(status_code=HTTPStatus.NO_CONTENT.value) in the clear endpoint
  4. You removed the TODO, but didn't add type hints (e.g. -> FramesClient) for the v3io clients getters

Also note the PR has conflicts with upstream, please pull latest development

@Michaelliv
Copy link
Contributor Author

  1. staticmethod -> classmethod

What is the point of this? a class method is wrong here.

@Hedingber Hedingber merged commit e80ccd3 into mlrun:development Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants