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

Add custom info field to ExternalResourceInfo #6115

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

hamersaw
Copy link
Contributor

Why are the changes needed?

Syncs IDL with Union.

What changes were proposed in this pull request?

Adds a custom info field to ExternalResourceInfo. This is an extension of the ExternalId field and can be used to pass plugin specific context or identifiers. We will use this to pass fast task worker assignment.

How was this patch tested?

Tested locally.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

N/A

Docs link

N/A

andrewwdye and others added 2 commits December 18, 2024 08:43
This change adds a new `FastTaskAssignment` message type to fast task protos, which is used to send actor assignment info to admin via task events.

While here I also hardened linting on our protos and updates existing protos to conform. This matches the restrictions we have in place in unionai/cloud/idl

- [x] Add and run unittests
- [x] Run locally and verify worker assignment info comes through in task events

```
❯ curl -s http://localhost:8088/api/v1/task_executions/flytesnacks/development/f0dcb5e7cae6d47acaf3/n0\?limit\=100 | jq '.taskExecutions[0].closure.metadata.externalResources'
[
  {
    "externalId": "",
    "index": 0,
    "retryAttempt": 0,
    "phase": "UNDEFINED",
    "cacheStatus": "CACHE_DISABLED",
    "logs": [],
    "customInfo": {
      "assignedWorker": "example-2b377271ee80",
      "environmentId": "flytesnacks_development_example_c183cb6c9868d96"
    }
  }
]

❯ curl -s http://localhost:8088/api/v1/task_executions/flytesnacks/development/f0dcb5e7cae6d47acaf3/n1\?limit\=100 | jq '.taskExecutions[0].closure.metadata.externalResources'
[
  {
    "externalId": "",
    "index": 0,
    "retryAttempt": 0,
    "phase": "UNDEFINED",
    "cacheStatus": "CACHE_DISABLED",
    "logs": [],
    "customInfo": {
      "assignedWorker": "example-2b377271ee80",
      "environmentId": "flytesnacks_development_example_c183cb6c9868d96"
    }
  }
]
```

One merged, bring to cloud. Will be a no-op initially, but we can use this in a few dependent tasks to surface logs or look up other fast task worker info.

Should this change be upstreamed to OSS (flyteorg/flyte)? If not, please uncheck this box, which is used for auditing. Note, it is the responsibility of each developer to actually upstream their changes. See [this guide](https://unionai.atlassian.net/wiki/spaces/ENG/pages/447610883/Flyte+-+Union+Cloud+Development+Runbook/#When-are-versions-updated%3F).
- [ ] To be upstreamed to OSS

ref COR-1581

Add custom info field to ExternalResourceInfo
Adds a custom info field to `ExternalResourceInfo`. This is an extension of the [ExternalId](https://github.com/unionai/flyte/blob/2b6dfcaedab7ed6a7606d1434211086d064c6560/flyteidl/protos/flyteidl/event/event.proto#L269) field and can be used to pass plugin specific context or identifiers. We will use this to pass fast task worker assignment.

N/A

Just merge, not used yet

Should this change be upstreamed to OSS (flyteorg/flyte)? If not, please uncheck this box, which is used for auditing. Note, it is the responsibility of each developer to actually upstream their changes. See [this guide](https://unionai.atlassian.net/wiki/spaces/ENG/pages/447610883/Flyte+-+Union+Cloud+Development+Runbook/#When-are-versions-updated%3F).
- [x] To be upstreamed to OSS

ref COR-1581

Closes #393

Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw hamersaw force-pushed the union/upstream-33bca79f0 branch from 5427d3b to 8e6a343 Compare December 18, 2024 14:43
@hamersaw hamersaw marked this pull request as ready for review December 18, 2024 14:44
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 16.66667% with 20 lines in your changes missing coverage. Please review.

Project coverage is 36.98%. Comparing base (87ef4a0) to head (8e6a343).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
flyteidl/gen/pb-go/flyteidl/event/event.pb.go 15.78% 16 Missing ⚠️
...in/pkg/repositories/transformers/task_execution.go 25.00% 2 Missing and 1 partial ⚠️
...propeller/pkg/controller/nodes/task/transformer.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6115      +/-   ##
==========================================
- Coverage   36.98%   36.98%   -0.01%     
==========================================
  Files        1318     1318              
  Lines      132449   132473      +24     
==========================================
+ Hits        48989    48993       +4     
- Misses      79207    79226      +19     
- Partials     4253     4254       +1     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.05% <25.00%> (-0.01%) ⬇️
unittests-flytecopilot 30.99% <ø> (ø)
unittests-flytectl 62.29% <ø> (ø)
unittests-flyteidl 7.24% <15.78%> (+<0.01%) ⬆️
unittests-flyteplugins 53.85% <ø> (ø)
unittests-flytepropeller 42.60% <0.00%> (-0.01%) ⬇️
unittests-flytestdlib 55.17% <ø> (ø)

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.

@hamersaw hamersaw merged commit 3c03cff into master Dec 18, 2024
51 of 52 checks passed
@hamersaw hamersaw deleted the union/upstream-33bca79f0 branch December 18, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants