Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Doc Hub proposal #303

Merged
merged 43 commits into from
Nov 7, 2022
Merged

Doc Hub proposal #303

merged 43 commits into from
Nov 7, 2022

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Jun 23, 2022

Signed-off-by: Kevin Su [email protected]

TL;DR

https://docs.google.com/document/d/1wvCvM6pLQ17Zr0DcpS9es2vOFQsNeOaNN99ufGw8O1U/edit#heading=h.w5wnz0kbiild

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

  • Add new entity (descriptionEntity)
  • Add description entity to taskSpec and workflowSpec. These two specs are only used in the create request.
  • Add GetDescriptionEntity and ListDescriptionEntities endpoints

Tracking Issue

flyteorg/flyte#531

Follow-up issue

NA

Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw marked this pull request as draft June 24, 2022 19:29
@pingsutw pingsutw marked this pull request as draft June 24, 2022 19:29
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #303 (8068970) into master (446b575) will not change coverage.
The diff coverage is n/a.

❗ Current head 8068970 differs from pull request most recent head fd49fac. Consider uploading reports for the commit fd49fac to get more accurate results

@@           Coverage Diff           @@
##           master     #303   +/-   ##
=======================================
  Coverage   75.46%   75.46%           
=======================================
  Files          18       18           
  Lines        1174     1174           
=======================================
  Hits          886      886           
  Misses        237      237           
  Partials       51       51           
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
clients/go/admin/authtype_enumer.go 27.27% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pingsutw added 2 commits June 28, 2022 02:00
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

A great start! Should we also "enhance" the parameter descriptions in the same PR? allowing labels... etc.

protos/flyteidl/admin/task.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/description_entity.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/description_entity.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/description_entity.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/description_entity.proto Outdated Show resolved Hide resolved
@katrogan
Copy link
Contributor

katrogan commented Sep 9, 2022

hey @pingsutw is this proposal ready to be reviewed?

@pingsutw
Copy link
Member Author

@katrogan yes, could you take a look, please?
Update:

  • Add tags to task/workflow/execution CreateRequest
  • Add descriptionEntity to task/workflow/launchPlan, so we can get entitySpec and descriptionEntity in one RPC call by using getRequest or ListRequest.

protos/flyteidl/admin/description_entity.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/execution.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/launch_plan.proto Outdated Show resolved Hide resolved
protos/flyteidl/core/identifier.proto Outdated Show resolved Hide resolved
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
// DescriptionEntity contains detailed description for the task/workflow.
// Documentation could provide insight into the algorithms, business use case, etc.
message DescriptionEntity {
// id represents the unique identifier of the description entity.
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to add an identifier resource type for description entities?

@@ -51,6 +55,9 @@ message TaskList {
message TaskSpec {
// Template of the task that encapsulates all the metadata of the task.
core.TaskTemplate template = 1;

// Represents the specification for Description.
DescriptionEntity description_entity = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always going to be populated in requests to Get and List tasks?

Copy link
Member Author

@pingsutw pingsutw Oct 13, 2022

Choose a reason for hiding this comment

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

TaskSpec is only used for TaskCreateRequest, and we use Task for get and list tasks

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! thanks for explaining

wild-endeavor
wild-endeavor previously approved these changes Oct 20, 2022
protos/flyteidl/admin/description_entity.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/task.proto Outdated Show resolved Hide resolved
protos/flyteidl/admin/workflow.proto Outdated Show resolved Hide resolved
// Fetch a :ref:`ref_flyteidl.admin.DescriptionEntity` object.
rpc GetDescriptionEntity (flyteidl.admin.ObjectGetRequest) returns (flyteidl.admin.DescriptionEntity) {
option (google.api.http) = {
get: "/api/v1/description_entities/{id.project}/{id.domain}/{id.name}/{id.version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to put the resource type in the url as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, id refers to a NamedEntityIdentifier which doesn't have a version.. what is this supposed to point to?

Copy link
Member Author

@pingsutw pingsutw Oct 21, 2022

Choose a reason for hiding this comment

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

yes, we should add resource_type.

cc @EngHabu one question. I got an error when adding {id.resource_type} to the GetDescriptionEntity endpoint.

$ make generate
....
+ mockery -dir=gen/pb-go/flyteidl/service/ -all -output=clients/go/admin/mocks
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
+ mockery -dir=gen/pb-go/flyteidl/datacatalog/ -name=DataCatalogClient -output=clients/go/datacatalog/mocks
Generating mock for: DataCatalogClient in file: clients/go/datacatalog/mocks/DataCatalogClient.go
go generate ./...
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType
Error parsing file:  /Users/kevin/git/flyteidl/gen/pb-go/flyteidl/service/admin.pb.gw.go:2743:29: undeclared name: ResourceType

However, it works if I add {resource_type} to ListDescriptionEntities endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

we use core.Identifier in ObjectGetRequest instead of NamedEntityIdentifier. there is a version in core.Identifier

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
wild-endeavor
wild-endeavor previously approved these changes Oct 25, 2022
Signed-off-by: Kevin Su <[email protected]>
@@ -19,7 +19,7 @@ enum ResourceType {
// Encapsulation of fields that uniquely identifies a Flyte resource.
message Identifier {
// Identifies the specific type of resource that this identifier corresponds to.
ResourceType resource_type = 1;
core.ResourceType resource_type = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

@evalsocket mind taking a look at this? do you have any ideas why this generates bad code?

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw merged commit 6627432 into master Nov 7, 2022
@pingsutw pingsutw deleted the doc-hub branch November 7, 2022 18:13
bstadlbauer pushed a commit to bstadlbauer/flyteidl that referenced this pull request Nov 14, 2022
* Add description entity

Signed-off-by: Kevin Su <[email protected]>

* Add id

Signed-off-by: Kevin Su <[email protected]>

* wip

Signed-off-by: Kevin Su <[email protected]>

* few update

Signed-off-by: Kevin Su <[email protected]>

* update service

Signed-off-by: Kevin Su <[email protected]>

* update service

Signed-off-by: Kevin Su <[email protected]>

* Add description entity to task and workflow

Signed-off-by: Kevin Su <[email protected]>

* update des entity

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* typo

Signed-off-by: Kevin Su <[email protected]>

* address comment

Signed-off-by: Kevin Su <[email protected]>

* update idl

Signed-off-by: Kevin Su <[email protected]>

* list description entity

Signed-off-by: Kevin Su <[email protected]>

* make generate

Signed-off-by: Kevin Su <[email protected]>

* make generate

Signed-off-by: Kevin Su <[email protected]>

* Update service name

Signed-off-by: Kevin Su <[email protected]>

* update endpoint

Signed-off-by: Kevin Su <[email protected]>

* update endpoint

Signed-off-by: Kevin Su <[email protected]>

* remove create_description_entity endpoint

Signed-off-by: Kevin Su <[email protected]>

* Add description to task/workflow

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>

* address comments

Signed-off-by: Kevin Su <[email protected]>

* address comments

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix test

Signed-off-by: Kevin Su <[email protected]>

* Add id.resource_type

Signed-off-by: Kevin Su <[email protected]>

* undeclared name: ResourceType

Signed-off-by: Kevin Su <[email protected]>

* update wrong code manually

Signed-off-by: Kevin Su <[email protected]>

* Fixed tests

Signed-off-by: Kevin Su <[email protected]>

* Fixed tests

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Yee Hing Tong <[email protected]>
Signed-off-by: Bernhard <[email protected]>
eapolinario pushed a commit that referenced this pull request Sep 8, 2023
* Add description entity

Signed-off-by: Kevin Su <[email protected]>

* Add id

Signed-off-by: Kevin Su <[email protected]>

* wip

Signed-off-by: Kevin Su <[email protected]>

* few update

Signed-off-by: Kevin Su <[email protected]>

* update service

Signed-off-by: Kevin Su <[email protected]>

* update service

Signed-off-by: Kevin Su <[email protected]>

* Add description entity to task and workflow

Signed-off-by: Kevin Su <[email protected]>

* update des entity

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* typo

Signed-off-by: Kevin Su <[email protected]>

* address comment

Signed-off-by: Kevin Su <[email protected]>

* update idl

Signed-off-by: Kevin Su <[email protected]>

* list description entity

Signed-off-by: Kevin Su <[email protected]>

* make generate

Signed-off-by: Kevin Su <[email protected]>

* make generate

Signed-off-by: Kevin Su <[email protected]>

* Update service name

Signed-off-by: Kevin Su <[email protected]>

* update endpoint

Signed-off-by: Kevin Su <[email protected]>

* update endpoint

Signed-off-by: Kevin Su <[email protected]>

* remove create_description_entity endpoint

Signed-off-by: Kevin Su <[email protected]>

* Add description to task/workflow

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>

* address comments

Signed-off-by: Kevin Su <[email protected]>

* address comments

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix test

Signed-off-by: Kevin Su <[email protected]>

* Add id.resource_type

Signed-off-by: Kevin Su <[email protected]>

* undeclared name: ResourceType

Signed-off-by: Kevin Su <[email protected]>

* update wrong code manually

Signed-off-by: Kevin Su <[email protected]>

* Fixed tests

Signed-off-by: Kevin Su <[email protected]>

* Fixed tests

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Yee Hing Tong <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants