Add python API for programmatic access#54
Conversation
|
@rohan-uiuc thanks for the PR. We like the idea of a python API, and the example usage is much appreciated. As a first step, I will make changes to your PR to refactor some stuff (noticed there is some duplication like the ModelConfig, and clean up the lint errors). Stay tuned. |
|
@amrit110 that sounds good, thank you! |
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #54 +/- ##
===========================================
+ Coverage 79.06% 82.88% +3.82%
===========================================
Files 4 11 +7
Lines 554 777 +223
===========================================
+ Hits 438 644 +206
- Misses 116 133 +17
🚀 New features to boost your workflow:
|
…lm-inference into rohan-uiuc-programmatic_access
…elper.py, remove empty cli/_utils.py
…hild helper classes for CLI
…tch for all CLI commands
amrit110
left a comment
amrit110
left a comment
There was a problem hiding this comment.
Very neat refactoring to use helper base classes, and inherit for the api and cli packages.
…ng around the API, creates confusion
vec_inf/api/_models.py
Outdated
|
|
||
|
|
||
| @dataclass | ||
| class ModelInfo: |
There was a problem hiding this comment.
I don't think that this state should live in the api namespace. My understanding is that most of them are the responsibility of the launcher and not particularly specific to either a python or cli entrypoint. Can we move them elsewhere?
vec_inf/api/client.py
Outdated
| Error if there was an error retrieving the status. | ||
| """ | ||
| try: | ||
| status_cmd = f"scontrol show job {slurm_job_id} --oneliner" |
There was a problem hiding this comment.
This kind of logic will be duplicated regardless of the entrypoint. Can we contain this logic in the slurm communication class?
vec_inf/api/_helper.py
Outdated
| from vec_inf.shared._models import ModelType | ||
|
|
||
|
|
||
| class APILaunchHelper(LaunchHelper): |
There was a problem hiding this comment.
Do we need to subclass this or can we push the client-type relevant logic into the client itself and use the same launcher class across all clients?
for more information, see https://pre-commit.ci
…lm-inference into rohan-uiuc-programmatic_access
vec_inf/cli/_helper.py
Outdated
| ) | ||
| import vec_inf.client._utils as utils | ||
| from vec_inf.cli._models import MODEL_TYPE_COLORS, MODEL_TYPE_PRIORITY | ||
| from vec_inf.client._config import ModelConfig |
There was a problem hiding this comment.
I don't think it makes sense to implement inheritance here. Currently we are importing all of the "private" functionality from the client just to make it work in the cli. If the cli just uses the client object then it has a much more abstract ".launch()", ".shutdown()" interface rather than deep coupling with the client implementation.
…ry excepts in api.py, update client tests accordingly
for more information, see https://pre-commit.ci
XkunW
force-pushed
the
programmatic_access
branch
from
19677df to
5ef4e1f
Compare
vec_inf/cli/_cli.py
Outdated
| MetricsResponseFormatter, | ||
| StatusResponseFormatter, | ||
| ) | ||
| from vec_inf.client._models import LaunchOptions, LaunchOptionsDict |
There was a problem hiding this comment.
Consider changing imports from private modules. Should these be exposed explicitly alongside the client if the expectation is that client users will need them (either internal users (cli) or external client users)?
…LI to cli/_utils.py
for more information, see https://pre-commit.ci
5 participants
PR Type
Feature
Short Description
sharedpackage.Tests Added
API functionality tests for listing models, launching models, checking status, and waiting for readiness