-
Notifications
You must be signed in to change notification settings - Fork 74
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
Parallelism UI improvements #421
Conversation
A preview of f5106cb is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/421 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
81ccead
to
ae1b728
Compare
3ab5de8
to
f771851
Compare
9931d88
to
a0241af
Compare
1. Displays sub-applications inline 2. Adds additional tooling on hierarchy of traces 3. A few other miscellaneous fixes around highlighting/table spacing Note that we change from having the sequence ID in the URL to having the point in the highlighted state in the URL. This allows us to have multiple applications at once. The code can be a little messy, and there are a few more places we should probably have this distinction made, but for now this really improves the experience with parallelism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to a0241af in 1 minute and 42 seconds
More details
- Looked at
1868
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. telemetry/ui/src/components/routes/app/AppView.tsx:164
- Draft comment:
Consider checking ifsearchParams.get('sequence_location')
is not null or undefined before parsing it withJSON.parse
to avoid potential runtime errors. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. telemetry/ui/src/components/routes/AppList.tsx:209
- Draft comment:
There's a commented-out line for filteringrootAppsToDisplay
. Ensure this is intentional and not left by mistake. - Reason this comment was not posted:
Confidence changes required:50%
TheAppListTable
component has a commented-out line that filtersrootAppsToDisplay
. This might be intentional, but it's worth noting in case it was left unintentionally.
3. telemetry/ui/src/components/routes/app/AnnotationsView.tsx:193
- Draft comment:
Ensure that settinghighlightedSequence
tocurrentEditingAnnotationContext.sequenceId
is intentional whencurrentEditingAnnotationContext
is undefined, as it defaults toundefined
. - Reason this comment was not posted:
Confidence changes required:50%
InAnnotationsView
, thehighlightedSequence
prop is set tocurrentEditingAnnotationContext.sequenceId
. However, ifcurrentEditingAnnotationContext
is undefined, it defaults toundefined
. This might be intentional, but it's worth noting in case it was overlooked.
4. telemetry/ui/src/components/routes/app/AnnotationsView.tsx:174
- Draft comment:
Ensure that settinghighlightedSequence
tocurrentEditingAnnotationContext.sequenceId
is intentional whencurrentEditingAnnotationContext
is undefined, as it defaults toundefined
. - Reason this comment was not posted:
Confidence changes required:50%
InAnnotationsView
, thehighlightedSequence
prop is set tocurrentEditingAnnotationContext.sequenceId
. However, ifcurrentEditingAnnotationContext
is undefined, it defaults toundefined
. This might be intentional, but it's worth noting in case it was overlooked.
5. telemetry/ui/src/components/routes/app/AppView.tsx:263
- Draft comment:
Double-check the logic for enablingcurrentFocusStepsData
query to ensure it aligns with the intended behavior, especially the conditioncurrentFocusAppID !== appID && currentFocusAppID !== undefined
. - Reason this comment was not posted:
Confidence changes required:50%
InAppView
, thecurrentFocusStepsData
query is enabled based oncurrentFocusAppID !== appID && currentFocusAppID !== undefined
. This logic might be correct, but it's worth double-checking to ensure it aligns with the intended behavior.
Workflow ID: wflow_nDiJuXmUy37yNsDW
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
a0241af
to
f5106cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on f5106cb in 16 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_B8052Abi3FUqA5ft
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
[Short description explaining the high-level reason for the pull request]
Changes
How I tested this
Notes
Checklist
Important
Enhance parallelism UI by improving step navigation, sub-application handling, and insights display across multiple components.
AppListTable
inAppList.tsx
to display all apps, not just root apps.AppView
inAppView.tsx
to handleSequenceLocation
for step navigation.AnnotationsView
inAnnotationsView.tsx
to supportSequenceLocation
for annotations.RecursionDepthPadding
andSelfLoadingSubApplicationContainer
inStepList.tsx
for better sub-application handling.WaterfallPiece
inStepList.tsx
to handle sub-actions.InsightSubTable
inInsightsView.tsx
to useSequenceLocation
for insights.useLocationParams
inutils.tsx
for consistent URL parameter handling.GraphBuilder
inburr/core/__init__.py
.useParams
import inAppView.tsx
.This description was created by for f5106cb. It will automatically update as commits are pushed.