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

[SDK] Add docstring for Train API #2075

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

andreyvelich
Copy link
Member

Related: #2013.

I added valid docstring for train API and added print message for users to provide a feedback.

Also, I renamed:
HuggingFaceTrainParams -> HuggingFaceTrainerParams
train_parameters -> trainer_parameters

We need to update SDK examples once we merge this change.

/assign @StefanoFioravanzo @kubeflow/wg-training-leads @deepanker13
/hold for review

Signed-off-by: Andrey Velichkevich <[email protected]>
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coveralls
Copy link

coveralls commented Apr 19, 2024

Pull Request Test Coverage Report for Build 8760737620

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.009%) to 35.306%

Files with Coverage Reduction New Missed Lines %
pkg/controller.v1/mpi/mpijob_controller.go 2 81.05%
Totals Coverage Status
Change from base Build 8733644470: -0.009%
Covered Lines: 4378
Relevant Lines: 12400

💛 - Coveralls

Signed-off-by: Andrey Velichkevich <[email protected]>
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thank you!
I left a comment about alpha stage.

/lgtm

)

print(
"Thank you for using `train` API for LLMs fine-tuning. This feature is in alpha stage "
Copy link
Member

Choose a reason for hiding this comment

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

What is the definition of the alpha stage in SDK?
If we say that this feature is still alpha stage, defininig criteria to graduate them to beta and GA would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know that @StefanoFioravanzo is working on this as part of this effort: kubeflow/community#709. When we have some time we can discuss what is the graduation steps for our new features.

Also, we do have criteria for Kubeflow components here: https://www.kubeflow.org/docs/started/support/#application-status, but it is not related to the components features.

Copy link
Member

Choose a reason for hiding this comment

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

But, both (kubeflow/community#709 and https://www.kubeflow.org/docs/started/support/#application-status) seem to be for components, not SDK.

This PR seems to introduce a dedicated graduation cycle for SDK separated from the application above, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tenzen-y If you check @StefanoFioravanzo 's issue he also says this:

With "component" I not only mean an entire project/repo but also significant features within those projects (think about the new Fine Tune APIs for LLM).

So we also consider on how we should define graduation criteria for our new big features.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I have missed that sentence. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Yes we need to resume the component/feature maturity discussion. Thank you @andreyvelich for adding that warning!

@johnugeorge
Copy link
Member

/hold cancel

@google-oss-prow google-oss-prow bot merged commit 86e0df1 into kubeflow:master Apr 24, 2024
38 checks passed
@andreyvelich andreyvelich deleted the sdk-doc-train-api branch April 24, 2024 18:26
ckyuto pushed a commit to ckyuto/training-operator that referenced this pull request Apr 26, 2024
* [SDK] Add docstring for Train API

Signed-off-by: Andrey Velichkevich <[email protected]>

* Update a few args

Signed-off-by: Andrey Velichkevich <[email protected]>

---------

Signed-off-by: Andrey Velichkevich <[email protected]>
Signed-off-by: Weiyu Yen <[email protected]>
ckyuto pushed a commit to ckyuto/training-operator that referenced this pull request Apr 26, 2024
* [SDK] Add docstring for Train API

Signed-off-by: Andrey Velichkevich <[email protected]>

* Update a few args

Signed-off-by: Andrey Velichkevich <[email protected]>

---------

Signed-off-by: Andrey Velichkevich <[email protected]>
Signed-off-by: Weiyu Yen <[email protected]>
johnugeorge pushed a commit to johnugeorge/training-operator that referenced this pull request Apr 28, 2024
* [SDK] Add docstring for Train API

Signed-off-by: Andrey Velichkevich <[email protected]>

* Update a few args

Signed-off-by: Andrey Velichkevich <[email protected]>

---------

Signed-off-by: Andrey Velichkevich <[email protected]>
johnugeorge pushed a commit to johnugeorge/training-operator that referenced this pull request Apr 28, 2024
* [SDK] Add docstring for Train API

Signed-off-by: Andrey Velichkevich <[email protected]>

* Update a few args

Signed-off-by: Andrey Velichkevich <[email protected]>

---------

Signed-off-by: Andrey Velichkevich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants