[WIP][flytepropeller][flyteagent] Rename Agent Service To Connector Service#6320
[WIP][flytepropeller][flyteagent] Rename Agent Service To Connector Service#6320Future-Outlier wants to merge 2 commits intomasterfrom
Conversation
Signed-off-by: Future-Outlier <eric901201@gmail.com>
Code Review Agent Run #476623Actionable Suggestions - 8
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
AI Code Review powered by |
Future-Outlier
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6320 +/- ##
==========================================
- Coverage 58.49% 57.90% -0.60%
==========================================
Files 937 774 -163
Lines 71088 57321 -13767
==========================================
- Hits 41583 33189 -8394
+ Misses 26353 21639 -4714
+ Partials 3152 2493 -659
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Future-Outlier <eric901201@gmail.com>
There was a problem hiding this comment.
todo:
- make enabled-plugins: agent-service to connector service
- implement the merge config funciton in
GetConfig - test in internal cluster
- discuss how to merge the webapi config with @pingsutw
Changelist by BitoThis pull request implements the following key changes.
|
| Message string | ||
| LogLinks []*flyteIdl.TaskLog | ||
| CustomInfo *structpb.Struct | ||
| ConnectorError *admin.AgentError |
There was a problem hiding this comment.
The field name AgentError has been renamed to ConnectorError in ResourceWrapper struct. This change seems to be part of a larger refactoring from 'agent' to 'connector' terminology, but the corresponding field in the Resource struct (line 838 in agent.pb.go) is still named AgentError. This inconsistency might cause issues when mapping between these structures.
Code suggestion
Check the AI-generated fix before applying
| ConnectorError *admin.AgentError | |
| AgentError *admin.AgentError |
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| } | ||
|
|
||
| func (p *Plugin) watchAgents(ctx context.Context, agentService *core.AgentService) { | ||
| func (p *Plugin) watchConnectors(ctx context.Context, connectorService *core.AgentService) { |
There was a problem hiding this comment.
The function name has been changed from watchAgents to watchConnectors, but there's a parameter type mismatch. The parameter connectorService is still of type *core.AgentService instead of *core.ConnectorService. This could lead to confusion and potential issues if the code expects a different service type.
Code suggestion
Check the AI-generated fix before applying
| func (p *Plugin) watchConnectors(ctx context.Context, connectorService *core.AgentService) { | |
| func (p *Plugin) watchConnectors(ctx context.Context, connectorService *core.ConnectorService) { |
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| } | ||
|
|
||
| func RegisterAgentPlugin(agentService *core.AgentService) { | ||
| func RegisterConnectorPlugin(connectorService *core.AgentService) { |
There was a problem hiding this comment.
The function parameter connectorService is of type *core.AgentService but based on the context, it should likely be *core.ConnectorService. This inconsistency could lead to type mismatch issues when the function is called.
Code suggestion
Check the AI-generated fix before applying
| func RegisterConnectorPlugin(connectorService *core.AgentService) { | |
| func RegisterConnectorPlugin(connectorService *core.ConnectorService) { |
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| // SetSupportedTaskType set supportTaskType in the connector service. | ||
| func (p *ConnectorService) SetSupportedTaskType(taskTypes []TaskType) { |
There was a problem hiding this comment.
The method signature has been changed from AgentService to ConnectorService, but there are references to this method in other files that might still be using AgentService. Consider updating all references to ensure consistency.
Code suggestion
Check the AI-generated fix before applying
| // SetSupportedTaskType set supportTaskType in the connector service. | |
| func (p *ConnectorService) SetSupportedTaskType(taskTypes []TaskType) { | |
| // SetSupportedTaskType set supportTaskType in the connector service. | |
| func (p *ConnectorService) SetSupportedTaskType(taskTypes []TaskType) { |
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| // Maps task types to their connectors. {TaskType: connectorDeploymentID} | ||
| ConnectorForTaskTypes map[string]string `json:"connectorForTaskTypes" pflag:"-,"` |
There was a problem hiding this comment.
The field name has been changed from AgentForTaskTypes to ConnectorForTaskTypes to better reflect its purpose. However, any code that previously referenced AgentForTaskTypes will now break. Consider adding a migration path or deprecation notice if this is a breaking change to existing configurations.
Code suggestion
Check the AI-generated fix before applying
| // Maps task types to their connectors. {TaskType: connectorDeploymentID} | |
| ConnectorForTaskTypes map[string]string `json:"connectorForTaskTypes" pflag:"-,"` | |
| // Maps task types to their connectors. {TaskType: connectorDeploymentID} | |
| ConnectorForTaskTypes map[string]string `json:"connectorForTaskTypes" pflag:"-,"` | |
| // Deprecated: Use ConnectorForTaskTypes instead. Maps task types to their agents. {TaskType: agentDeploymentID} | |
| AgentForTaskTypes map[string]string `json:"agentForTaskTypes" pflag:"-," deprecated:"use connectorForTaskTypes instead"` |
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return core.PhaseInfoFailure(pluginErrors.TaskFailedWithError, "failed to run the job.\n"+resource.Message, taskInfo), nil | ||
| case admin.State_RETRYABLE_FAILURE: | ||
| return core.PhaseInfoRetryableFailure(pluginErrors.TaskFailedWithError, "failed to run the job.\n"+resource.Message, taskInfo), nil | ||
| case admin.State_SUCCEEDED: |
There was a problem hiding this comment.
It looks like there's a missing implementation for handling the State_SUCCEEDED case in the Status function. The function should write outputs and return a success phase when the state is State_SUCCEEDED, similar to how it's done for the TaskExecution_SUCCEEDED phase. This could lead to tasks not completing properly when using the legacy state field.
Code suggestion
Check the AI-generated fix before applying
@@ -311,0 +312,7 @@
case admin.State_SUCCEEDED:
err = writeOutput(ctx, taskCtx, resource.Outputs)
if err != nil {
logger.Errorf(ctx, "failed to write output with err %s", err.Error())
return core.PhaseInfoUndefined, fmt.Errorf("failed to write output with err %s", err.Error())
}
return core.PhaseInfoSuccess(taskInfo), nil
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| func SetConfig(cfg *Config) error { | ||
| return connectorConfigSection.SetConfig(cfg) |
There was a problem hiding this comment.
The SetConfig function is trying to set a *Config type to the connectorConfigSection which expects a *ConnectorConfig. This type mismatch could lead to runtime errors. Consider updating the function to accept *ConnectorConfig instead.
Code suggestion
Check the AI-generated fix before applying
| func SetConfig(cfg *Config) error { | |
| return connectorConfigSection.SetConfig(cfg) | |
| func SetConfig(cfg *ConnectorConfig) error { | |
| return connectorConfigSection.SetConfig(cfg) |
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| }, | ||
| registry: agentRegistry, | ||
| }, nil | ||
| }, |
There was a problem hiding this comment.
The plugin initialization in newMockAsyncAgentPlugin() is missing required fields. The current implementation creates a Plugin with empty maps for clients but doesn't set the registry field which is used in the plugin's logic. Consider initializing the registry field with the agentRegistry that's already defined in the function.
Code suggestion
Check the AI-generated fix before applying
| }, | |
| }, | |
| registry: agentRegistry, |
Code Review Run #476623
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #d220e2Actionable Suggestions - 0Review Details
AI Code Review powered by |
3 participants
Tracking issue
Why are the changes needed?
Given there's a name collision between LLM agent and flyte agent, we are going to rename flyte agent to flyte connectors.
What changes were proposed in this pull request?
How was this patch tested?
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR renames the Flyte Agent Service to Flyte Connector Service to eliminate naming collisions and improve clarity. The changes update service identification across the codebase by replacing hard-coded values with dynamic references, while also updating type names, function signatures, package names, and documentation to enhance maintainability.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5