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

TAO API Integration #613

Draft
wants to merge 13 commits into
base: branch-23.11
Choose a base branch
from

Conversation

bsuryadevara
Copy link
Contributor

  • Created an API wrapper for the TAO REST API.
  • Added unit tests

@bsuryadevara bsuryadevara added non-breaking Non-breaking change feature request New feature or request 2 - In Progress labels Jan 17, 2023
@bsuryadevara bsuryadevara self-assigned this Jan 17, 2023
@bsuryadevara bsuryadevara marked this pull request as ready for review January 17, 2023 23:17
@bsuryadevara bsuryadevara requested a review from a team as a code owner January 17, 2023 23:17
@bsuryadevara bsuryadevara linked an issue Jan 30, 2023 that may be closed by this pull request
2 tasks
@bsuryadevara bsuryadevara changed the base branch from branch-23.01 to branch-23.03 February 6, 2023 18:55
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments, this PR needs:

  • Typing annotations for all methods
  • docstrings for public methods

from morpheus.pipeline.training.tao_client import vaildate_apikey


@pytest.mark.use_python
Copy link
Contributor

Choose a reason for hiding this comment

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

@pytest.mark.use_python is only needed if you have config in the test arguments.

Comment on lines 104 to 111
def generate_schema_url(url, ssl):
if url.startswith("http://") or url.startswith("https://"):
raise ValueError("URL should not include the scheme")

scheme = "https://" if ssl else "http://"
url = scheme + (url if url[-1] != "/" else url[:-1])

return url
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be using urllib for this instead of writing our own.

return url


def vaildate_apikey(apikey):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add typing information to all of the public functions.

return apikey


class TaoApiClient():
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this class pulled from somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TAO exposed REST endpoints, but not client API. So, I've created a wrapper around it.
https://docs.nvidia.com/tao/tao-toolkit/text/tao_toolkit_api/api_rest_api.html

return apikey


class TaoApiClient():
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this is the correct location for this file. This class isnt related to pipelines at all and is standalone. Would work better in the io directory


def authorize(self):

endpoint = f"{self._base_uri}/login/{self._apikey}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use urllib when generating URLs like this.


@pytest.mark.use_python
def get_tao_client():
mock_creds = {"user_id": "X20109876", "token": "TOkJJTkw6WkxKRDpNWk9ZOkRVN0o6"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these randomly generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, those are mock values.



@pytest.mark.use_python
def get_tao_client():
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a pytest fixture. See other uses of @pytest.fixture

@bsuryadevara
Copy link
Contributor Author

@drobison00 @mdemoret-nv This PR's feedback improvements require testing in the TAO toolkit API environment (K8s). It will affect my local morpheus/mrc conda environments, docker setup and nvidia-drivers. When everything is ready for the Integrated Training and Feedback feature, I will take care of this.

@mdemoret-nv mdemoret-nv changed the base branch from branch-23.03 to branch-23.11 July 14, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Non-breaking change
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

[FEA]: TAO API Client Wrapper
3 participants