-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat(weave): frontend to allow deletion of single call #1664
feat(weave): frontend to allow deletion of single call #1664
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=be219d3b162238f9af8e4f8f4dc5712f4ebf4e2b |
4fc23d1
to
f7faab2
Compare
@@ -417,6 +417,7 @@ export const browse3ContextGen = ( | |||
primaryDim | |||
)}&secondaryDim=${encodeURIComponent(secondaryDim)}`; | |||
}, | |||
refetchCalls: () => {}, |
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.
This does not belong in the Url context. I'll keep reading and suggest an alternative place
closePeek(); | ||
} else { | ||
// Not peeking (fullscreen) so navigate to the parent page | ||
const path = history.location.pathname.split('/'); |
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.
i'm not quite sure what the intention is here
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.
This is totally a hack, but when you are in fullscreen mode (which you can navigate to directly with a link) there is nothing to close once you have deleted the call you are looking at. So we have to navigate up the tree after deletion as the current page is no longer valid...
@@ -51,6 +51,7 @@ const WFDataModelFromTraceServerCallsOnlyProvider: FC = ({children}) => { | |||
return { | |||
useCall: tsWFDataModelHooks.useCall, | |||
useCalls: tsWFDataModelHooks.useCalls, | |||
useCallsDelete: tsWFDataModelHooks.useCallsDelete, |
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.
nit/question: useCallsDelete
follows a slightly different pattern (it returns a function). I wonder if there is a naming convention that could imply this pattern difference?
const getTsClient = useGetTraceServerClientContext(); | ||
const loadingRef = useRef(false); | ||
const [callRes, setCallRes] = | ||
useState<traceServerClient.TraceCallsQueryRes | null>(null); | ||
const deepFilter = useDeepMemo(filter); | ||
|
||
const [lastFetch, setLastFetch] = useState<number | null>(null); |
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.
this whole block might be cleaner if instead of relying on states to trigger hooks, we instead:
- Create a callback called
doFetch
that does the fetch and updates the state - Call
doFetch
inside the current useEffect
(by this point it is a pure refactor) refetch
is then literally just thedoFetch
method (:
@@ -191,6 +191,9 @@ export const CallsTable: FC<{ | |||
|
|||
// Fetch the calls | |||
const calls = useCallsForQuery(entity, project, effectiveFilter); | |||
if (calls.refetch) { | |||
baseContext.refetchCalls = calls.refetch; |
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.
This pattern would not be ideal in situations where there are 2 calls table on the screen at the same time (as there is when you open an eval). In general, it is not good practice to assign some state like this to a global symbol. Two comments:
- If you wanted to do a pattern like this, you would want to have a react context provider somewhere above that allows children to "register" and "unregister" refetch triggers.
- But even that is a little odd - and you might want to reach for a reducer pattern with listeners.
Still, all these options are a bit odd - you really want someway to "broadcast" changes.
Got to run to dinner. Will respond later
@@ -191,6 +191,9 @@ export const CallsTable: FC<{ | |||
|
|||
// Fetch the calls | |||
const calls = useCallsForQuery(entity, project, effectiveFilter); | |||
if (calls.refetch) { | |||
baseContext.refetchCalls = calls.refetch; |
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.
This pattern would not be ideal in situations where there are 2 calls table on the screen at the same time (as there is when you open an eval). In general, it is not good practice to assign some state like this to a global symbol. Two comments:
- If you wanted to do a pattern like this, you would want to have a react context provider somewhere above that allows children to "register" and "unregister" refetch triggers.
- But even that is a little odd - and you might want to reach for a reducer pattern with listeners.
Still, all these options are a bit odd - you really want someway to "broadcast" changes.
Let's see... in the best case scenario, upon deletion, two events occur:
- All components on the page which include the deleted element adjust
- If the primary view is of the deleted element, we navigate away.
Furthermore, when we have renaming, we want similar behavior: all cases of the name change are reflected immediately.
I can think of 4 families of solutions:
- Don't do anything and just let the next user-invoked navigation reflect the changes.
- Hard refresh the whole page.
- Broadcast change events to listening components to manually reflect the change.
- Note: this is closest to your current implementation (where "reflect" is facilitated via a refresh)
- Automatically update any query hooks with the updated information.
- The challenge here is not to be overly aggressive causing full-page reloads. Might be widespread refectors
- are there others?...
What i don't like about #3 is that we put the responsibility on every rendering component to "listen" listen for changes. This is a lot a coupling in the design.
#4 is probably the best option across the board if done correctly. How does this relate to polling?
I wonder if the same mechanism which is hypothetically triggered in a poll event should be triggered after a mutation event? Where such a mechanism is: "refresh all active queries, without re-triggering loading/empty states". Yeah, doing this correctly might require a more widespread refactor.
Ok, so options...:
- (low-cost version of No more weave-internal #4) I wonder if a low-cost change to
OptionalTraceServerClientContextProvider
that allows mutation hooks to refresh the client would be jarring? Might be worth a try. - Do what you are doing now, but with a proper context
- Somehow have the other call hooks react to the delete event
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.
excellent comment, will work through these and have a soln by tue
6c23b18
to
99bbd22
Compare
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
8c088a6
to
95bb392
Compare
…hub.com:wandb/weave into griffin/deleted-at-frontend-no-bulk-clean
@@ -48,8 +48,19 @@ const projectIdFromParts = ({ | |||
project: string; | |||
}) => `${entity}/${project}`; | |||
|
|||
// Trace server client keys that are promises | |||
type TraceServerClientPromiseKeys = { |
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.
Oh god! 🥷
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.
SHIP IT!
Single call deletion path. Also works for evaluations.
https://wandb.atlassian.net/browse/WB-18627
depends on: