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

H-1323: Add /workers page, update flow run routes #4479

Merged
merged 13 commits into from
May 22, 2024
Merged

Conversation

CiaranMn
Copy link
Member

🌟 What is the purpose of this PR?

The PR introduces a /workers page which lists all active worker runs. These may be 'goals' which are research-driven flows (the /goals page to be introduced in the next PR), or other flows.

It also updates the routes for viewing:

  1. Flow definitions to /@[namespace]/flows/[id]
  2. Worker runs (flows or goals) to /@[namespace]/workers/[id]

These routes have essentially the same UI for now so they share a component. I've also updated the context providers so that we poll for all flow runs with fewer details (that doesn't require the event history) on a slower schedule, and the single selected flow run more frequently.

πŸ”— Related links

πŸ” What does this change?

As well as the main changes above:

  • Throws errors without a cause from run-workflow-run.ts because the cause causes Temporal to not fail the workflow properly
  • Update the persistFlow activity to use the provided webId instead of always the user

Pre-Merge Checklist πŸš€

🚒 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

πŸ“œ Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

πŸ•ΈοΈ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

πŸ›‘ What tests cover this?

  • None yet.

@CiaranMn CiaranMn requested a review from benwerner01 May 17, 2024 17:45
@github-actions github-actions bot added area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team type/eng > backend Owned by the @backend team area/apps labels May 17, 2024
Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 21.23%. Comparing base (4c0810c) to head (fc3bd24).

Files Patch % Lines
libs/@local/hash-backend-utils/src/flows.ts 0.00% 16 Missing ⚠️
...sh-ai-worker-ts/src/workflows/run-flow-workflow.ts 0.00% 10 Missing ⚠️
.../hash-isomorphic-utils/src/flows/frontend-paths.ts 0.00% 7 Missing ⚠️
apps/hash-api/src/ai/infer-entities-websocket.ts 0.00% 2 Missing ⚠️
...ctivities/flow-activities/persist-flow-activity.ts 0.00% 1 Missing ⚠️
...orphic-utils/src/flows/example-flow-definitions.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4479      +/-   ##
==========================================
- Coverage   21.52%   21.23%   -0.29%     
==========================================
  Files         451      454       +3     
  Lines       14953    15156     +203     
  Branches     2216     2267      +51     
==========================================
  Hits         3218     3218              
- Misses      11694    11897     +203     
  Partials       41       41              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

benwerner01
benwerner01 previously approved these changes May 22, 2024
Copy link
Member

@benwerner01 benwerner01 left a comment

Choose a reason for hiding this comment

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

Thanks Ciaran, these changes look good to me πŸ‘

Comment on lines -390 to +378
throw new Error(errorMessage, {
cause: {
code: StatusCode.FailedPrecondition,
message: errorMessage,
contents: [{ flow }],
},
});
throw new Error(errorMessage);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can discuss this internally rather than in a thread, but these outputs have been useful for me in the past when debugging errors in flows - but it makes sense to remove them if they're leading to the flow not being failed correctly.

I suppose I can use the getFlowRuns resolver in the meantime.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can actually restore these, I thought it was causing temporal to not fail but it's actually because I wasn't using their special ApplicationError (added in the follow up goals PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this in the next PR, b211df2

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, thanks!

Base automatically changed from cm/convert-browser-inference-to-flows to main May 22, 2024 15:14
@CiaranMn CiaranMn dismissed benwerner01’s stale review May 22, 2024 15:14

The base branch was changed.

@CiaranMn CiaranMn requested a review from benwerner01 May 22, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/apps area/libs Relates to first-party libraries/crates/packages (area) type/eng > backend Owned by the @backend team type/eng > frontend Owned by the @frontend team
Development

Successfully merging this pull request may close these issues.

None yet

2 participants