-
Notifications
You must be signed in to change notification settings - Fork 373
View: Show Retried Logs #3013
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
View: Show Retried Logs #3013
Conversation
dragonstyle
left a comment
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.
one comment -looks good!
* extract the shared logic into a hook, using data from store instead of already processed objects * make it easier to debug data on Alt+click (including a list of duplicates)
src/inspect_ai/_view/www/src/app/samples-panel/samples-grid/SamplesGrid.tsx
Outdated
Show resolved
Hide resolved
src/inspect_ai/_view/www/src/app/samples-panel/SamplesPanel.tsx
Outdated
Show resolved
Hide resolved
| const currentDirTaskIds = new Set( | ||
| currentDirLogFilesMaybeSkippedRetries.map((f) => f.task_id), | ||
| ); | ||
| let count = currentDirLogFilesMaybeSkippedRetries.length; |
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.
the bottom-right corner count will now match the Tasks panel (number of deduplicated log files) instead of different counts between the 2 panels
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.
Are you saying the footer will now display the number of log files that match vs the number of samples (when in the sample view)? If so, that seems wrong to me - that lower right corner number is intended to display the total number of items being displayed in the grid control (given filtering, etc...).
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.
ah, I didn't change that part, but LogListFooter displays different info in different situations that I was not aware of:
- if eval set is already finished, it's already showing number of samples (same on main branch and this PR ... but my PR needs a fix to reflect Show Retried Logs)
- fix sample count in lower right corner
- if eval set is still running (
totalTaskCount !== completedTaskCount), it toggles to show progress bar (same on main branch and this PR ... but the count in this PR was already reflecting Show Retried Logs when I wrote my comment => hopefully no further fixes needed)
dragonstyle
left a comment
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.
Thanks for all this work - this is a tricky problem to get right! I've made some suggestions.
This PR contains:
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Show Retried Logsto be able to see them regardless of eval set running statusDoes this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Promise.allto load eval set info faster