-
Notifications
You must be signed in to change notification settings - Fork 2
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
Client overhaul to match server routes #73
base: launch-v1
Are you sure you want to change the base?
Conversation
launch/client.py
Outdated
gpu_type=gpu_type, | ||
default_callback_url=default_callback_url, | ||
) | ||
return json.loads(response.resopnse.data) |
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.
ah typo
query_params=query_params, | ||
skip_deserialization=True, | ||
) | ||
return response |
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.
need to parse
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 skipped deserialization because there are sometimes inconsistencies between return types and the auto-generated response types. They're finicky sometimes and imo the extra typing we get isn't worth the hassle.
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 think wrapping purely around the REST APIs does make more sense, but I'm wondering what would make the most sense for releasing to users. We made the migration to v1 REST API routes relatively painless for users by maintaining pretty much the same Python API, and I wonder if we would want to be a bit less aggressive and simply decorate the old Python API as @deprecated
and unsupported in a few minor releases.
Yup those are my thoughts too - mark the existing functions as |
No description provided.