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

[feature] Adding description to pipeline version #6256

Closed
casassg opened this issue Aug 6, 2021 · 15 comments
Closed

[feature] Adding description to pipeline version #6256

casassg opened this issue Aug 6, 2021 · 15 comments
Assignees
Labels
area/backend area/frontend kind/feature lifecycle/stale The issue / pull request is stale, any activities remove this label.

Comments

@casassg
Copy link
Contributor

casassg commented Aug 6, 2021

Feature Area

/area frontend
/area backend

What feature would you like to see?

While using KFP, we lack a way to add descriptions to the pipeline version. This would be useful for example to track metadata of the version uploaded (like for example what git sha was this version generated from).

What is the use case or pain point?

Easier integration into CD systems for continous compilation and deployment of pipeline versions.

Is there a workaround currently?

Hard to do, you can modify the description of the main pipeline and append new lines for each version, that said it may eventually overload that column and make it hard to read vs just having it on the version itself.


Love this idea? Give it a 👍. We prioritize fulfilling features with the most 👍.

@Bobgy
Copy link
Contributor

Bobgy commented Aug 6, 2021

Thank you for explaining the use case clearly!
This seems helpful to many.
So welcome contributions!

@casassg
Copy link
Contributor Author

casassg commented Aug 6, 2021

@Bobgy any pointers to how to add a field? Seems I can should add it

message PipelineVersion {
but then not sure how to propagate this across server api and such (or if it needs it somewhere else) or it needs some DB migration. (Im less familiar w the inner workings of KFP sorry)

@Bobgy
Copy link
Contributor

Bobgy commented Aug 6, 2021

Backend code: https://github.com/kubeflow/pipelines/tree/master/backend
Backend api pipeline version proto:

message PipelineVersion {

Regenerate code after updating proto, https://github.com/kubeflow/pipelines/tree/master/backend/api#auto-generation-of-go-client-and-swagger-definitions
Pipeline server:
version, err := s.resourceManager.CreatePipelineVersion(request.Version, pipelineFile, common.IsPipelineVersionUpdatedByDefault())

Resource manager:
func (r *ResourceManager) CreatePipelineVersion(apiVersion *api.PipelineVersion, pipelineFile []byte, updateDefaultVersion bool) (*model.PipelineVersion, error) {

Pipeline store (db layer):
CreatePipelineVersion(*model.PipelineVersion, bool) (*model.PipelineVersion, error)

UI pipeline details page:
https://github.com/kubeflow/pipelines/blob/master/frontend/src/pages/PipelineDetails.tsx

Here are most of related code you need to touch, feel free to ask for help if you have further questions!

@casassg
Copy link
Contributor Author

casassg commented Aug 7, 2021

Started backend changes in #6261 . I regenerated most code and seems backend is working (mostly just ran unit tests to ensure data was being written to DB and could be read back). Missing to run some integration tests (need to spend more time to add integration tests).

@Bobgy
Copy link
Contributor

Bobgy commented Aug 19, 2021

Thank you for implementing the backend API change! Welcome continuing the UI integration, but you can always leave that to a different contributor too.

@casassg
Copy link
Contributor Author

casassg commented Aug 19, 2021

I'm happy to contribute to UI as well. Using this as a learning opportunity.

However if someone else comes w this issue and wants to implement it, let me know! (I will add to my backlog once backend gets merged, but I dont know when I can focus on this)

google-oss-robot pushed a commit that referenced this issue Aug 19, 2021
…6256 (#6261)

* start adding description to version

* add description to resource manager

* remove description from pipeline default version

* update pipeline_store

* add some tests to ensure its being passed around

* update frontend generated api

* run auto formatter

* add comment to frontend readme about running npm run format

* add missing api converter step
@casassg
Copy link
Contributor Author

casassg commented Aug 19, 2021

Start frontend changes, found out that I missed an endpoint for backend (the HTTP upload one, as those dont use grpc)

@casassg
Copy link
Contributor Author

casassg commented Aug 19, 2021

Will also start a separate PR to update kfp sdk

google-oss-robot pushed a commit that referenced this issue Aug 24, 2021
#6256 (#6405)

* add version description to pipelineVersionList

* remove extra tests and reduce flex for version name

* add tooltip and mock version data
@Bobgy
Copy link
Contributor

Bobgy commented Aug 30, 2021

There's a report the SDK change caused a regression:

    pipeline_id=pipeline_id)
  File "/tmpfs/BUILD_ENV/.venv/lib/python3.7/site-packages/kfp/_client.py", line 1143, in upload_pipeline_version
    description=description,
  File "/tmpfs/BUILD_ENV/.venv/lib/python3.7/site-packages/kfp_server_api/api/pipeline_upload_service_api.py", line 209, in upload_pipeline_version
    return self.upload_pipeline_version_with_http_info(uploadfile, **kwargs)  # noqa: E501
  File "/tmpfs/BUILD_ENV/.venv/lib/python3.7/site-packages/kfp_server_api/api/pipeline_upload_service_api.py", line 265, in upload_pipeline_version_with_http_info
    " to method upload_pipeline_version" % key
kfp_server_api.exceptions.ApiTypeError: Got an unexpected keyword argument 'description' to method upload_pipeline_version

@Bobgy
Copy link
Contributor

Bobgy commented Aug 30, 2021

I'll be taking a look

@ConverJens
Copy link
Contributor

@Bobgy This mismatch is present in the latest released sdk package 1.7.2. Could you build a new patch version with this fixed?

@casassg
Copy link
Contributor Author

casassg commented Sep 1, 2021

btw, fixed it #6472

google-oss-robot pushed a commit that referenced this issue Sep 7, 2021
…#6393)

* add description to upload_pipeline_description http endpoint

* add test for pipeline upload server description

* add description to new pipeline version

* show pipeline version in pipeline detail page

* make description optional on UI + show version descriptio or pipeline description

* remove unused reference

* revert wrong change of ref

* updated on should be pipeline version

* add more versions to mock-backend

* show pipeline and version description at the same time

* add tests for UI

* show pipeline version always
google-oss-robot pushed a commit that referenced this issue Oct 25, 2021
#6472)

* add description optionally

* add release notes

* add new line

* capture if description is not accepted by server

* add todo comment

* add better description api missing exception
@casassg
Copy link
Contributor Author

casassg commented Oct 27, 2021

Merged. I believe now the only missing part is for this to be released on the next KFP release

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Mar 2, 2022
Copy link

This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/frontend kind/feature lifecycle/stale The issue / pull request is stale, any activities remove this label.
Projects
Status: Closed
Development

No branches or pull requests

4 participants