-
Notifications
You must be signed in to change notification settings - Fork 700
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
[SDK] Train API #1962
[SDK] Train API #1962
Conversation
Pull Request Test Coverage Report for Build 7477774579
💛 - Coveralls |
ff86df6
to
8cbe61e
Compare
/hold cancel |
/assign @andreyvelich |
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.
Thank you for this @deepanker13 !
8cbe61e
to
c59b0b0
Compare
|
a64b243
to
e4df199
Compare
c6467de
to
95173aa
Compare
8493b38
to
cd811cd
Compare
@andreyvelich I have a reason to keep the download dir field, as there will be a single place where we define the default value and that same value will be passed through the code flow, else we will have to verify the values are same or not through the entire code flow. |
@@ -1,40 +1,51 @@ | |||
from abstract_model_provider import modelProvider |
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.
Let's name the directory storage_initiailizer
rather than storage_init_container
to be consistent with init container name ?
WDYT @deepanker13 ?
@deepanker13 Can you just have 3 constant variables in
And then just re-use these constants in SDK and |
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, we are ready to merge this PR.
Thanks again @deepanker13 for all of this work!
/lgtm
/assign @johnugeorge @tenzen-y for the final review.
b62ad9b
to
b2dbcd0
Compare
@andreyvelich @johnugeorge @tenzen-y thanks for all the help |
Awesome work @deepanker13 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepanker13, johnugeorge The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...
format, will close the issue(s) when PR gets merged):Partially Fixes #1945
Checklist: