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

Add Pending State and State Message in Webapi Agent #399

Closed

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Sep 4, 2023

TL;DR

As title.

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

For Databricks agent service, we need to have a pending state.
Now we can see what's happening in the pending state!

image

Follow-up issue

flyteorg/flyteidl#440
flyteorg/flytekit#1834

Future Outlier added 2 commits September 4, 2023 21:49
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier
Copy link
Member Author

@pingsutw Just fixed my error, please review!
Thanks a lot!

Future Outlier added 2 commits September 5, 2023 16:26
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier
Copy link
Member Author

Please approve it again, thanks a lot!

Signed-off-by: Future Outlier <[email protected]>
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2598c96) 62.73% compared to head (9e97713) 64.23%.
Report is 3 commits behind head on master.

❗ Current head 9e97713 differs from pull request most recent head 90cff0b. Consider uploading reports for the commit 90cff0b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
+ Coverage   62.73%   64.23%   +1.49%     
==========================================
  Files         156      156              
  Lines       13173    10649    -2524     
==========================================
- Hits         8264     6840    -1424     
+ Misses       4284     3187    -1097     
+ Partials      625      622       -3     
Flag Coverage Δ
unittests ?

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

Files Coverage Δ
go/tasks/pluginmachinery/core/phase.go 21.50% <ø> (-1.27%) ⬇️
go/tasks/plugins/webapi/agent/plugin.go 75.15% <100.00%> (+9.10%) ⬆️

... and 135 files with indirect coverage changes

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

@Future-Outlier
Copy link
Member Author

Please review again, thanks a lot!

pingsutw
pingsutw previously approved these changes Sep 5, 2023
go/tasks/plugins/webapi/agent/plugin.go Outdated Show resolved Hide resolved
@Future-Outlier
Copy link
Member Author

Just tested it in Databricks agent service, please review it, thanks a lot!

@kumare3
Copy link
Contributor

kumare3 commented Sep 9, 2023

@pingsutw / @Future-Outlier can we add more states like initializing etc, all the ones we already have in the task phases?

Also have messages for each phase sent by the agent

@@ -162,6 +162,8 @@ func (p Plugin) Status(ctx context.Context, taskCtx webapi.StatusContext) (phase
taskInfo := &core.TaskInfo{}

switch resource.State {
case admin.State_PENDING:
return core.PhaseInfoQueuedWithTaskInfo(core.DefaultPhaseVersion, "Job is PENDING", taskInfo), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the message. This message does not convey any information to the user. Can the agent send this message

Copy link
Member Author

Choose a reason for hiding this comment

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

@kumare3

func PhaseInfoInitializing(t time.Time, version uint32, reason string, info *TaskInfo) PhaseInfo {
pi := phaseInfo(PhaseInitializing, version, nil, info, false)
pi.reason = reason
return pi
}

Is this solution suitable?

Future Outlier added 2 commits September 14, 2023 08:22
@Future-Outlier
Copy link
Member Author

@kumare3, @pingsutw just fix it, please review!

Future Outlier added 2 commits September 14, 2023 16:16
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier Future-Outlier changed the title Add Pending State in Webapi Agent Add Pending State and State Message in Webapi Agent Sep 14, 2023
Future Outlier added 2 commits September 27, 2023 07:26
@eapolinario
Copy link
Contributor

eapolinario commented Oct 3, 2023

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to . Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

edit: Re-opening these PRs as the automation failed.

@eapolinario eapolinario closed this Oct 3, 2023
@eapolinario eapolinario reopened this Oct 3, 2023
@eapolinario
Copy link
Contributor

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4133. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

@eapolinario eapolinario closed this Oct 3, 2023
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.

4 participants