[flyte-core] Flyte Connection #5126

pingsutw · 2024-03-27T20:10:02Z

Tracking issue

#3936

Why are the changes needed?

It will allow users to pass different API_KEY to the chatGPT agent from flytepropeller

What changes were proposed in this pull request?

Agent client will read the secret from env or local file, and pass it to the agent server.

How was this patch tested?

local sandbox

Setup process

Install flytekit and flyteidl

pip install "git+https://github.com/flyteorg/flytekit.git@76dfef0c89615bb28dfa1ead932544150d9c2cde"
pip install "git+https://github.com/flyteorg/flyte.git@d96975d0bc590b57e9205d81f4de9595a4bd1708#subdirectory=flyteidl"

Update local config

Add below config to flyte-single-binary-local.yamlm and Add your API key to local env

export FLYTE_SECRET_OPENAI_API_KEY=<YOUR_OPENAI_API_KEY>

externalResources:
  connections:
    chatgpt:
      secrets:
        openai_api_key: OPENAI_API_KEY
      configs:
        openai_organization: org-P2rdnZQry4Fw7Ak3vSpXEIrx

Run Flyte cluster locally

cd flyte
pyflyte serve agent
make compile
flyte start --config flyte-single-binary-local.yaml

Run a ChatGPT task

pyflyte run --remote flyte-example/chatgpt_example.py my_chatgpt_job --message hello

from flytekit import workflow, Secret
from flytekitplugins.chatgpt import ChatGPTTask

chatgpt_small_job = ChatGPTTask(
    name="3.5-turbo",
    openai_organization="org-P2rdnZQry4Fw7Ak3vSpXEIrx",
    connection="chatgpt",
    chatgpt_config={
        "model": "gpt-3.5-turbo",
        "temperature": 0.7,
    },
)


@workflow
def my_chatgpt_job(message: str) -> str:
    message = chatgpt_small_job(message=message)
    return message

Screenshots

Secret not found in propeller

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

Related PRs

flyteorg/flytekit#2297

Docs link

NA

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

codecov · 2024-03-27T20:16:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.98%. Comparing base (ce5eb03) to head (ad0a88f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5126      +/-   ##
==========================================
+ Coverage   60.99%   66.98%   +5.98%     
==========================================
  Files         794      278     -516     
  Lines       51475    12638   -38837     
==========================================
- Hits        31397     8465   -22932     
+ Misses      17186     3549   -13637     
+ Partials     2892      624    -2268

Flag	Coverage Δ
unittests-datacatalog	`69.31% <ø> (ø)`
unittests-flyteadmin	`?`
unittests-flytecopilot	`17.79% <ø> (ø)`
unittests-flytectl	`68.03% <ø> (ø)`
unittests-flyteidl	`79.04% <ø> (ø)`
unittests-flyteplugins	`?`
unittests-flytepropeller	`?`
unittests-flytestdlib	`65.59% <ø> (+0.02%)`	⬆️

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.

noahjax · 2024-03-28T17:25:47Z

@pingsutw Would it be possible to merge the work of this PR with that of my PR? It seems like both include secrets in requests made to the agent, but this PR goes a step farther by resolving the value of secrets. On the other hand, my PR includes more details about which user is executing a task, which is also very useful for the agent to have.

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

flytepropeller/pkg/controller/nodes/task/connectionmanager/config.go

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

flytepropeller/pkg/controller/nodes/task/handler.go

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

pmahindrakar-oss · 2024-05-11T15:07:18Z

flyteidl/protos/flyteidl/admin/matchable_resource.proto

@@ -36,6 +36,9 @@ enum MatchableResource {

  // Controls how to select an available cluster on which this execution should run.
  CLUSTER_ASSIGNMENT = 7;
+
+  // Configures the task connection to be used by the agent to connect to external systems.
+  EXTERNAL_RESOURCE = 8;


We could extend this to be used by regular tasks too right ?

What do you mean by extending here? something like

@task(external_resource=...) # or @task(connection=...)

pmahindrakar-oss · 2024-05-11T15:08:56Z

flyteidl/protos/flyteidl/core/security.proto

+    // Flyte will use the default connection in the project-domain settings, but users
+    // still be able to override it by specifying the connection in the task decorator.
+    // +optional
+    string connection = 4;


Could we make a note of the order of overrides as its just not project-domain settings alone. Also can we add details on how this should map to values in external resource attributes

pmahindrakar-oss · 2024-05-14T00:47:57Z

flyteidl/protos/flyteidl/core/security.proto

@@ -45,6 +45,14 @@ message Secret {
    MountType mount_requirement = 4;
 }

+message Connection {
+    // The credentials to use for the connection, such as API keys, OAuth2 tokens, etc.
+    map<string, string> secrets = 1;


Can we also add comment on the format of what the key and what the value is and how they map to what the agent expects

pmahindrakar-oss · 2024-05-14T00:48:07Z

flyteidl/protos/flyteidl/core/security.proto

+    map<string, string> secrets = 1;
+
+    // The configuration to use for the connection, such as the endpoint, account name, etc.
+    map<string, string> configs = 2;


Similalrly for the configs

pmahindrakar-oss · 2024-05-14T00:53:28Z

flyteplugins/go/tasks/plugins/webapi/agent/plugin.go

@@ -97,6 +99,24 @@ func (p Plugin) Create(ctx context.Context, taskCtx webapi.TaskExecutionContextR
 	taskCategory := admin.TaskCategory{Name: taskTemplate.Type, Version: taskTemplate.TaskTypeVersion}
 	agent, isSync := getFinalAgent(&taskCategory, p.cfg, p.agentRegistry)

+	connection := flyteIdl.Connection{}
+	if taskTemplate.SecurityContext != nil && taskTemplate.SecurityContext.Connection != "" {
+		conn, ok := taskCtx.TaskExecutionMetadata().GetExternalResourceAttributes().GetConnections()[taskTemplate.SecurityContext.Connection]


Can we use getters for taskTemplate.SecurityContext.Connection and would be cleaner to define this as connectionKey may be as we are not defining the connection in the security context but just a ref/key. May be connectionRef might also be ok

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

pingsutw added 3 commits March 26, 2024 17:25

Add secret

dfffe71

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

Create mock class

fa7573c

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

update mock

0e235e1

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

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. documentation Improvements or additions to documentation enhancement New feature or request security Issues related to Security improvements labels Mar 27, 2024

feat: Add Secrets support to webapi plugin requests

60754bc

pingsutw added 3 commits April 3, 2024 16:39

wip

1e82896

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

merged master

18e91a2

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

secretPtr

42bbe73

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

pingsutw changed the title ~~[WIP] Pass secret to the agent server~~ Pass secret to the agent server Apr 4, 2024

pingsutw and others added 7 commits April 5, 2024 11:46

wip

a61fa8c

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

Add connection

d895c71

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

update proto

61263ad

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

merged master

0e69505

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

lint

e928f35

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

cleanup

0b80b41

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

wip

249fbe9

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

pingsutw commented Apr 17, 2024

View reviewed changes

flytepropeller/pkg/controller/nodes/task/connectionmanager/config.go Outdated Show resolved Hide resolved

pingsutw added 8 commits April 29, 2024 15:09

wip

d525cf9

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

go generate

79302e5

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

fix tests

df830ea

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

test

9574f59

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

more tests

7dedb8f

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

fix test

23357c5

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

lint

307c06c

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

Merge branch 'master' of github.com:flyteorg/flyte into agent-secret

9429ac2

lint

aecfaa5

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

pingsutw commented Apr 30, 2024

View reviewed changes

flytepropeller/pkg/controller/nodes/task/handler.go Outdated Show resolved Hide resolved

pingsutw added 2 commits May 1, 2024 22:48

merged master

7afda69

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

Add interface

0e25e5b

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

pingsutw changed the title ~~Pass secret to the agent server~~ [flyte-core] Flyte Connection May 7, 2024

pingsutw added 12 commits May 10, 2024 17:26

wip

1f01450

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

Merge branch 'master' of github.com:flyteorg/flyte into agent-secret

0808ec0

Add ExternalResourceAttributes

b4f264c

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

nit

466c61c

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

update test

b19daea

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

update test

090a10b

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

fix test

d96975d

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

fix test

22966d0

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

fix test

54e5cc8

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

fix test

a8b7d06

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

Add more tests

78b7dfc

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

install tabulate

bf9b39f

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

pmahindrakar-oss reviewed May 14, 2024

View reviewed changes

pingsutw added 6 commits May 16, 2024 01:21

Address comment

f7e79b4

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

nit

d12a889

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

Merge branch 'master' of github.com:flyteorg/flyte into agent-secret

ff3ece0

rename connection_ref

0c0f723

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

make generate

39093e1

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

merged master

ad0a88f

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

pingsutw marked this pull request as draft August 16, 2024 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flyte-core] Flyte Connection #5126

[flyte-core] Flyte Connection #5126

pingsutw commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

noahjax commented Mar 28, 2024

pmahindrakar-oss May 11, 2024

pingsutw May 15, 2024

pmahindrakar-oss May 11, 2024

pmahindrakar-oss May 14, 2024

pmahindrakar-oss May 14, 2024

pmahindrakar-oss May 14, 2024

[flyte-core] Flyte Connection #5126

Are you sure you want to change the base?

[flyte-core] Flyte Connection #5126

Conversation

pingsutw commented Mar 27, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Related PRs

Docs link

codecov bot commented Mar 27, 2024 • edited Loading

Codecov Report

noahjax commented Mar 28, 2024

pmahindrakar-oss May 11, 2024

Choose a reason for hiding this comment

pingsutw May 15, 2024

Choose a reason for hiding this comment

pmahindrakar-oss May 11, 2024

Choose a reason for hiding this comment

pmahindrakar-oss May 14, 2024

Choose a reason for hiding this comment

pmahindrakar-oss May 14, 2024

Choose a reason for hiding this comment

pmahindrakar-oss May 14, 2024

Choose a reason for hiding this comment

pingsutw commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading