8000 Remove SDK by eoinfennessy · Pull Request #2657 · kubeflow/trainer · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove SDK #2657

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jun 9, 2025
Merged

Remove SDK #2657

merged 11 commits into from
Jun 9, 2025

Conversation

eoinfennessy
Copy link
Contributor

What this PR does / why we need it:

The Trainer SDK has been moved to kubeflow/sdk. This PR removes the SDK, SDK models generation, and fixes references to the SDK within this repo.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

  • Docs included if any changes are user facing

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

kramaranya
kramaranya suggested changes Jun 5, 2025
View reviewed changes
Copy link
Contributor
@kramaranya kramaranya left a comment

Choose a reason for hiding this comment

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

Great, thanks @eoinfennessy
We also need to update Makefile, github workflows, pre-commit hooks and documentation, such as README, CONTRIBUTING

@google-oss-prow Google OSS Prow
Copy link

@kramaranya: changing LGTM is restricted to collaborators

In response to this:

Great, thanks @eoinfennessy
We also need to update Makefile, github workflows, pre-commit hooks and documentation, such as README, CONTRIBUTING

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@eoinfennessy
Copy link
Contributor Author

@kramaranya, thanks. I will update pre-commit config and docs.

I made some changes to the Makefile and GH workflows. Is there anything else that should be done?

andreyvelich
andreyvelich reviewed Jun 5, 2025
View reviewed changes
Copy link
Member
@andreyvelich andreyvelich 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 for this @eoinfennessy!
/assign @astefanutti @Electronic-Waste @tenzen-y @kubeflow/wg-training-leads

hack/python-sdk/gen-sdk.sh
@@ -1,62 +0,0 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

We need to move this generation script under kubeflow/sdk/hack.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the old script that was used to generate models in trainer/sdk/kubeflow/trainer/models. Since we moved models to trainer/api/python_api/kubeflow_trainer_api/models, we now use the new gen-api.sh script instead. So I think gen-api.sh should stay in the trainer repo, and gen-sdk.sh should be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you are right, we are no longer need OpenAPI generator to create Kubeflow SDK clients, since we don't use them.

@andreyvelich andreyvelich added this to the v2.0 milestone Jun 5, 2025
@kramaranya
Copy link
Contributor

Thanks @eoinfennessy for the update! I think .gitignore and release README still need to be updated

@andreyvelich
Copy link
Member

@eoinfennessy Please can you rebase this PR, so we can merge it ?

@kramaranya
Copy link
Contributor

@eoinfennessy could you update the jupyter notebooks? + rebase the pr

@eoinfennessy
Remove SDK
3f48cf0 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Fix e2e test
0aea1fe 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Remove unnecessary pip install ./sdk for unit tests
116d691 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Remove sdk from pre-commit config
f2f93c6 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Update docstring for generate target
d8904a3 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Update CONTRIBUTING.md
2a15d9b 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Remove .gitignore for OpenAPI Generator CLI JAR
0f0c462 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
README.md: Add link to kubeflow/sdk + some minor edits
1b5639d 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Add TODO to repurpose SDK release info if needed
9707da1 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@eoinfennessy
Copy link
Contributor Author

@andreyvelich @kramaranya the PR has been rebased.

tenzen-y
tenzen-y reviewed Jun 9, 2025
View reviewed changes
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
/lgtm
/approve

.github/workflows/test-e2e.yaml
@@ -42,7 +42,7 @@ jobs:
pip install papermill==2.6.0 jupyter==1.1.1 ipykernel==6.29.5

echo "Install Kubeflow SDK"
pip install ./sdk
pip install git+https://github.com/kubeflow/sdk.git@main#subdirectory=python
Copy link
Member

Choose a reason for hiding this comment

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

Just noted, @andreyvelich we need to merge SDK adding PR to the main branch in SDK repository before we cut new 2.0 RC version in this repository.

Copy link
Member

Choose a reason for hiding this comment

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

@tenzen-y I think, we've already done it. Isn't?
https://github.com/kubeflow/sdk/tree/main/python

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I missed that.
Thx

@google-oss-prow google-oss-prow bot added the lgtm label Jun 9, 2025
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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

@kramaranya
Copy link
Contributor

/hold

@tenzen-y
Copy link
Member
tenzen-y commented Jun 9, 2025

/hold

@kramaranya Why do we need to prevent the merge?

@kramaranya
Copy link
Contributor

@kramaranya Why do we need to prevent the merge?

@tenzen-y Sorry, just currently deciding with Eoin about adding the SDK installation step to the jupyter notebooks, same as here, wdyt?

@eoinfennessy
Add SDK installation step to all notebooks
b33fa8a 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@google-oss-prow google-oss-prow bot removed the lgtm label Jun 9, 2025
@eoinfennessy
Copy link
Contributor Author

@kramaranya I added SDK installation steps to all notebooks in b33fa8a

@kramaranya
Copy link
Contributor

Thanks a lot @eoinfennessy!!
/hold cancel

@tenzen-y
Copy link
Member
tenzen-y commented Jun 9, 2025

@kramaranya Why do we need to prevent the merge?

@tenzen-y Sorry, just currently deciding with Eoin about adding the SDK installation step to the jupyter notebooks, same as here, wdyt?

SGTM
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jun 9, 2025
@andreyvelich
Copy link
Member

@eoinfennessy @kramaranya we should have capability to override the installed SDK package before we run the Jupyter Notebooks.
We need to make sure that we build and run some of these Notebooks are part of: kubeflow/sdk#18
Which means, we should build SDK from source and run examples.

I would suggest that we keep SDK line commented in the Notebooks, so we can install the correct version when it is needed.

@kramaranya
Copy link
Contributor

I would suggest that we keep SDK line commented in the Notebooks, so we can install the correct version when it is needed.

Agree!

@eoinfennessy
Comment out SDK installation command in all notebooks
05e2940 
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
@google-oss-prow google-oss-prow bot removed the lgtm label Jun 9, 2025
@eoinfennessy
Copy link
Contributor Author

I would suggest that we keep SDK line commented in the Notebooks, so we can install the correct version when it is needed.
Thanks @andreyvelich! Done in 05e2940

andreyvelich
andreyvelich reviewed Jun 9, 2025
View reviewed changes
Copy link
Member
@andreyvelich andreyvelich 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 for this work @eoinfennessy!
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jun 9, 2025
@google-oss-prow google-oss-prow bot merged commit a4a3376 into kubeflow:master Jun 9, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreyvelich andreyvelich andreyvelich left review comments

@tenzen-y tenzen-y tenzen-y left review comments

@kramaranya kramaranya kramaranya requested changes

@Electronic-Waste Electronic-Waste Awaiting requested review from Electronic-Waste

@jinchihe jinchihe Awaiting requested review from jinchihe

Assignees

@andreyvelich andreyvelich

@tenzen-y tenzen-y

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@eoinfennessy @kramaranya @andreyvelich @tenzen-y
0