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

Artifact protos and related changes #4474

Merged
merged 61 commits into from
Dec 27, 2023
Merged

Artifact protos and related changes #4474

merged 61 commits into from
Dec 27, 2023

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Nov 23, 2023

This change introduces the artifact protobufs and related control plane changes. The actual service backing artifacts is still being worked on, and related Helm chart changes and such are not yet available.

Admin changes have been feature-gated. To turn on the changes (though again, they won't work without the backend service which is coming soon), add the following to your single binary configuration.

flyteadmin:
  featureGates:
    enableArtifacts: true

To enable the new cloudevents

cloudEvents:
  enable: true
  cloudEventVersion: v2

Summary of changes

  • IDL changes
    • New artifact related identifiers in core.
    • New artifact related messages and service
    • Artifact related fields in Variable and Parameter
    • Metadata field in Literal for tracking
    • New cloud events with additional information
  • Admin
    • Registration of launch plans, tasks, and workflows, will send signatures to the artifact service.
    • Execution manager will resolve any artifact queries before running an execution
  • Others
    • Change and combine postgres error handling logic into flytestdlib and call from admin and data catalog
    • Add a sandbox-only event processor to flytestdlib which can only be used in single binary.

wild-endeavor and others added 26 commits October 11, 2023 14:49
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Main pr to bring in the boilerplate code for artifact service into the main pr.

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
This PR adds the trigger concept.

Other changes:
* Forgot to make the index on artifactkey unique.
* Fix to create artifact, it wasn't pulling back out the artifact key.
* Updated the Artifact service to handle the passing of artifacts created by the events processor to the trigger handling components. Currently, and this is potentially a design flaw, the events processor has no way of contacting the trigger component directly.

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Most of the changes here are generated proto changes. Actual code changes are:
IDL
* Remove the supplemental fields in the CloudEventTaskExecution object and move them to CloudEventNodeExecution object.
* Remove some fields that the artifact service ended up not using (parent_node_execution and scheduled_at)

in the cloudevent publisher, change the code filling in of the aforementioned supplemental information to happen for node execution events instead of task execution events.
* Remove the deleted fields.

On the event handling side, move the logic to the handling of the node event instead of the task event.
Signed-off-by: Yee Hing Tong <[email protected]>
* lint fixes
* rename sandbox_utils without underscore

Signed-off-by: Yee Hing Tong <[email protected]>
Add additional handling of pgconn
* Change datacatalog logic to use flytestdlib function instead.
* Change flytestdlib constants to capitalize
* Add handling of the other type to Admin code directly

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: squiishyy <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
…lone artifact client and move into idl clientset, update local config file, remove a deprecated function (#4473)

Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor changed the title Artifacts shell Shell Nov 23, 2023
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Attention: 731 lines in your changes are missing coverage. Please review.

Comparison is base (0e35cab) 59.03% compared to head (80316c7) 58.10%.

Files Patch % Lines
...cloudevent/implementations/cloudevent_publisher.go 0.86% 342 Missing ⚠️
flyteadmin/pkg/manager/impl/execution_manager.go 15.38% 249 Missing and 4 partials ⚠️
flyteadmin/pkg/artifacts/registry.go 25.37% 50 Missing ⚠️
flyteadmin/pkg/manager/impl/launch_plan_manager.go 26.92% 17 Missing and 2 partials ⚠️
flyteadmin/pkg/async/cloudevent/factory.go 51.72% 14 Missing ⚠️
flyteadmin/pkg/rpc/adminservice/base.go 0.00% 14 Missing ⚠️
flyteadmin/pkg/manager/impl/task_manager.go 47.05% 8 Missing and 1 partial ⚠️
flyteadmin/pkg/common/flyte_url.go 0.00% 8 Missing ⚠️
flyteadmin/pkg/manager/impl/workflow_manager.go 61.90% 7 Missing and 1 partial ⚠️
...yteadmin/pkg/manager/impl/validation/validation.go 50.00% 4 Missing and 1 partial ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4474      +/-   ##
==========================================
- Coverage   59.03%   58.10%   -0.93%     
==========================================
  Files         622      626       +4     
  Lines       52823    53821     +998     
==========================================
+ Hits        31182    31272      +90     
- Misses      19150    20046     +896     
- Partials     2491     2503      +12     
Flag Coverage Δ
unittests 58.10% <15.68%> (-0.93%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eapolinario and others added 12 commits December 21, 2023 14:35
This reverts commit cfa4745.

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
This reverts commit 8ded67b.

Signed-off-by: Eduardo Apolinario <[email protected]>
This reverts commit 3968d40.

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor marked this pull request as ready for review December 23, 2023 05:48
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request housekeeping Issues that help maintain flyte and keep it tech-debt free labels Dec 23, 2023
@wild-endeavor wild-endeavor changed the title Shell Artifact protos and related changes Dec 25, 2023
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario eapolinario merged commit 4a7c3c0 into master Dec 27, 2023
45 of 47 checks passed
@wild-endeavor wild-endeavor mentioned this pull request Dec 28, 2023
wild-endeavor added a commit that referenced this pull request Jan 8, 2024
This is a follow up PR to the [first shell pr](#4474), which just brought in the non-artifact-service artifact changes.  This PR continues with additional smaller changes.

* Remove the `/data` prefix in the grpc gateway paths.
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor deleted the artifacts-shell branch January 10, 2024 16:26
katrogan pushed a commit that referenced this pull request Jan 12, 2024
This is a follow up PR to the [first shell pr](#4474), which just brought in the non-artifact-service artifact changes.  This PR continues with additional smaller changes.

* Remove the `/data` prefix in the grpc gateway paths.
Signed-off-by: Yee Hing Tong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request housekeeping Issues that help maintain flyte and keep it tech-debt free size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants