-
Notifications
You must be signed in to change notification settings - Fork 145
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
base: branch-23.11
Are you sure you want to change the base?
TAO API Integration #613
Conversation
bsuryadevara
commented
Jan 17, 2023
- Created an API wrapper for the TAO REST API.
- Added unit tests
There was a problem hiding this 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
tests/test_tao_client.py
Outdated
from morpheus.pipeline.training.tao_client import vaildate_apikey | ||
|
||
|
||
@pytest.mark.use_python |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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(): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(): |
There was a problem hiding this comment.
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}" |
There was a problem hiding this comment.
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"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these randomly generated?
There was a problem hiding this comment.
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(): |
There was a problem hiding this comment.
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
@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. |