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

docs(backend): improved backend README #11511

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

dandawg
Copy link
Contributor

@dandawg dandawg commented Jan 13, 2025

Description of your changes:
This PR makes the following improvements to the backend/README.md file, which are broadly aimed at making the file more user friendly (easier to follow and clearer to understand):

  1. Added an Overview section (intro) to make it more clear what the readme's purpose is.
  2. Added a Prerequisites section to help people know up-front what they need. (In the future, it may be good to give even more details here, for example on versions). The go-licences pre-req is particularly important to building the docker image--without it users wait for the build to fail, then troubleshoot.
  3. Organized content into major sections with more appropriate header levels.
  4. The Code Style section was in the previously in the middle of the 'building and testing' commands--I moved it to it's own Contributing section at the end.
  5. Fixed make all command to run from perspective the root (pipelines) directory (to be consistent with the other commands).
  6. This also clarifies what is happening when some commands are run to help understanding / prevent misunderstandings.

Checklist:

Signed-off-by: Daniel Dowler <[email protected]>
Copy link

Hi @dandawg. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@hbelmiro
Copy link
Contributor

/ok-to-test

Copy link

Approvals successfully granted for pending runs.

Copy link
Contributor

@hbelmiro hbelmiro 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 @dandawg.
I left a few comments.

backend/README.md Outdated Show resolved Hide resolved
backend/README.md Outdated Show resolved Hide resolved
backend/README.md Outdated Show resolved Hide resolved
backend/README.md Outdated Show resolved Hide resolved
dandawg and others added 4 commits January 14, 2025 12:10
Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>
Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>
Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>
Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>
@dandawg
Copy link
Contributor Author

dandawg commented Jan 14, 2025

Thank you @dandawg. I left a few comments.

Great suggestions. I added in the changes @hbelmiro

Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

/lgtm
/ok-to-test


## Prerequisites
Before you begin, ensure you have:
- Go programming language installed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Go programming language installed
- [Go installed](https://go.dev/doc/install)

I think typically you would say install "Go" and not "Go programming language" always nice to also link to the docs https://go.dev/doc/install

#### Deleting the Cluster

Run the following to delete the cluster:
Also, note that the config in the `make` command above sets the `ml-pipeline` `Deployment` (api server) to have 0 replicas. The intent is to replace it with a locally running API server for debugging and faster development. See the following steps to run the API server locally, and connect it to the KFP backend on your Kind cluster. Note that other backend components (for example, the persistence agent) may show errors until the API server is brought up and connected to the cluster.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Also, note that the config in the `make` command above sets the `ml-pipeline` `Deployment` (api server) to have 0 replicas. The intent is to replace it with a locally running API server for debugging and faster development. See the following steps to run the API server locally, and connect it to the KFP backend on your Kind cluster. Note that other backend components (for example, the persistence agent) may show errors until the API server is brought up and connected to the cluster.
> [!NOTE]
> The config in the `make` command above sets the `ml-pipeline` `Deployment` (api server) to have 0 replicas. The intent is to replace it with a locally running API server for debugging and faster development. See the following steps to run the API server locally, and connect it to the KFP backend on your Kind cluster. Note that other backend components (for example, the persistence agent) may show errors until the API server is brought up and connected to the cluster.

the above makes a prettier note rendering

@HumairAK
Copy link
Collaborator

left some comments but they are mostly nits, I'm happy to merge as is, if you'd like to follow up with the amendments, feel free!

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK

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

@google-oss-prow google-oss-prow bot merged commit ce3850a into kubeflow:master Jan 29, 2025
32 checks passed
mholder6 pushed a commit to mholder6/pipelines that referenced this pull request Jan 31, 2025
Signed-off-by: Helber Belmiro <[email protected]>

docs(backend): improved backend README (kubeflow#11511)

* improved backend README

Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

---------

Signed-off-by: Daniel Dowler <[email protected]>
Co-authored-by: Helber Belmiro <[email protected]>

fix(CI): Use the correct image registry for replacements in integration tests (kubeflow#11564)

* Use the correct image registry for replacements in integration tests

The image registry was changed to GitHub Container Registry in the 2.4
release.

Signed-off-by: mprahl <[email protected]>

* Print the pod logs when the pods fail to start in integration tests

Signed-off-by: mprahl <[email protected]>

* Fix the sample compilation in the API server container build

Signed-off-by: mprahl <[email protected]>

* Show the output when building the container images in CI

Signed-off-by: mprahl <[email protected]>

---------

Signed-off-by: mprahl <[email protected]>

feat(api): Add SemaphoreKey and MutexName fields to proto (kubeflow#11384)

Signed-off-by: ddalvi <[email protected]>
mholder6 pushed a commit to mholder6/pipelines that referenced this pull request Jan 31, 2025
Signed-off-by: Helber Belmiro <[email protected]>

docs(backend): improved backend README (kubeflow#11511)

* improved backend README

Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

* Update backend/README.md

Co-authored-by: Helber Belmiro <[email protected]>
Signed-off-by: Daniel Dowler <[email protected]>

---------

Signed-off-by: Daniel Dowler <[email protected]>
Co-authored-by: Helber Belmiro <[email protected]>

fix(CI): Use the correct image registry for replacements in integration tests (kubeflow#11564)

* Use the correct image registry for replacements in integration tests

The image registry was changed to GitHub Container Registry in the 2.4
release.

Signed-off-by: mprahl <[email protected]>

* Print the pod logs when the pods fail to start in integration tests

Signed-off-by: mprahl <[email protected]>

* Fix the sample compilation in the API server container build

Signed-off-by: mprahl <[email protected]>

* Show the output when building the container images in CI

Signed-off-by: mprahl <[email protected]>

---------

Signed-off-by: mprahl <[email protected]>

feat(api): Add SemaphoreKey and MutexName fields to proto (kubeflow#11384)

Signed-off-by: ddalvi <[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.

3 participants