-
Notifications
You must be signed in to change notification settings - Fork 156
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
Allow filtering samples by compound expressions including multiple scorers #1073
base: main
Are you sure you want to change the base?
Conversation
This is looking really great to me, and I love that we'll be able to support much more robust filtering! I believe that the right autocomplete experience can make this nearly as easy to use as the simple selector. I have some suggestions to get there:
Don't mean to flood with feedback, but this is definitely getting there and I'm looking forward to merging it!! |
Thank for the feedback! Implemented all suggestions, please take a look. |
(These check failures are not related to this PR and are related to ruff dependency version changes. They are now fixed on main so if you rebase against main they should go away - sorry!) |
Uses filtrex to support compound expressions that allow to filter samples by multiple scores at a time.
… likely to continue
Version 18.1.1 (20619.2.8.11.12). I only see it once I make the expression long and scroll with the mouse... |
One other question - rather than show the green feedback treatment once an expression is complete, maybe we should just apply the expression at that point since we know it will work? It would still result in some changes to filtering in cases where the filters didn't narrow the set (e.g or) but I think that would be worth getting an even smoother experience. |
My Safari version is slightly different (Version 18.2 (20620.1.16.11.8)), don't know if it's related or not. To be honest, debugging this kind of failure without being able to reproduce it would be quite hard. I decided to just remove the scroll bar. It's quite unusual for single-line text inputs to have scroll bars anyway. |
Agreed. Applying the filter expression immediately but only when it's valid seems like a good approach. Changed the behavior and added color-coding, which is hopefully noticeable enough to make the current state clear, but not so much as to be distracting. |
This is a great improvement over our current filtering. Nits:
I haven't looked closely at the code itself - LMK if you think that is ready to go and I can take a look (or just ping me whenever you think its good to go). |
Also note - good feature suggestion for error filtering here: https://inspectcommunity.slack.com/archives/C080ET25C81/p1736618476283839 |
Removed the green outline. It was an experiment, I wasn't sure how I feel about it myself. What is really important IMO is to give some indication whether the filter is applied or not. But graying out inactive filters is sufficient for that. Restored the secondary bar item order. Sorry I forgot to revert this when moving the filter to the left. I thought you wanted aggressive popups :) But maybe I misunderstood what you were suggesting earlier. It's true that now you would get a completion suggestion after I think we have a lot of opportunities to add more features to the new filter: integrate epoch filter, filter by sample metadata, add full-text transcript search, etc. I've added the one you suggested to my list, but we can do it later, right? No need to do everything in this PR, I think |
Sounds perfect to me!
+100! |
Disabled automatic suggestions in the middle (except after a dot). WDYT? |
Looks great to me. One final tiny nit - since the filter is often the first focusable element on the page, it gets focus by default (which triggers showing the autocompletion). What would you think about filtering this case (just using Taking a quick pass through the code now. |
Good catch. It was actually worse then that: for some reason I decided that the filter field should be focused whenever the filter changes externally, so the call was coming from inside the house. Fixed. And added |
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.
Look great having gone through the code. One open question and a few styling things specific to vscode. Could chat anytime in realtime if helpful!
[data-tooltip] { | ||
position: relative; | ||
} | ||
[data-tooltip]:hover::after { |
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 think this needs:
background: var(--bs-body-bg);
opacity: 1;
The body-bg is set dynamically in the case of VSCode (to match the vscode theme). Best test there is to open a log in vscode and toggle theme (usually just selecting a dark theme is enough to uncover issues)
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.
Beyond the tooltip, I think we need to set up the overall theme style (attached image shows what I'm talking bout). Currently we 'forward' vscode colors into css vars as needed in App.css here:
body[class^="vscode-"] {
--bs-border-radius: 0;
--bs-border-radius-lg: 0;
--bs-body-bg: var(--vscode-editor-background);
--bs-card-bg: var(--vscode-editor-background);
--bs-table-bg: var(--vscode-editor-background);
--bs-light-bg-subtle: var(--vscode-sideBar-background);
--bs-light-border-subtle: var(--vscode-sideBarSectionHeader-border);
--bs-body-color: var(--vscode-editor-foreground);
--bs-table-color: var(--vscode-editor-foreground);
--bs-accordion-btn-color: var(--vscode-editor-foreground);
--bs-emphasis-color: var(--vscode-editor-foreground);
--bs-navbar-brand-color: var(--vscode-editor-foreground);
--bs-navbar-brand-hover-color: var(--vscode-editor-foreground);
--bs-code-color: var(--vscode-editorInfo-foreground);
--bs-light: var(--vscode-sideBar-background);
--bs-btn-bg: var(--vscode-peekViewTitle-background);
--bs-primary: var(--vscode-banner-iconForeground);
--bs-nav-pills-link-active-bg: var(--vscode-banner-iconForeground);
--bs-secondary: var(--vscode-breadcrumb-foreground);
--bs-secondary-bg: var(--vscode-list-inactiveSelectionBackground);
--bs-border-color: var(--vscode-editorGroup-border);
--bs-card-border-color: var(--vscode-editorGroup-border);
--bs-warning-bg-subtle: var(--vscode-inputValidation-warningBackground);
--bs-warning-text-emphasis: var(--vscode-input-foreground);
--inspect-find-background: var(--vscode-editorWidget-background);
--inspect-find-foreground: var(--vscode-editorWidget-foreground);
--inspect-input-background: var(--vscode-input-background);
--inspect-input-foreground: var(--vscode-input-foreground);
--inspect-input-border: var(--vscode-input-border);
--inspect-diff-add-color: var(--vscode-diffEditor-insertedTextBackground);
--inspect-diff-remove-color: var(--vscode-diffEditor-removedTextBackground);
}
The process of discovering vscode colors is more art than science as they are not well documented. I usually just use the 'Open Webview Developer Tools' within vscode and browser the variables available to the viewer and try to select colors that seem correct + might have names that seem to be likely to be semantically related to what they'll represent in inspect.
Happy to chat more real time if useful...
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.
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.
Yes, good point! I replaced hard-coded values with CSS variables and fixed the dark theme. At least I hope I did.
Unfortunately I could not find the variables for the build-in selection colors and for the BS focus outline color (--inspect-focus-border-color
), so I had to specify those manually. But pretty much everything else is taken from the theme now.
{ tag: tags.number, color: "#164" }, | ||
{ tag: tags.keyword, color: "#708" }, | ||
{ tag: tags.function(tags.variableName), color: "#00c" }, | ||
]); |
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.
We should be using CSS vars (if we can) to allow vscode overrides to work properly. Alternatively, we could select middle ground hues of the colors (e.g. the red is currently pretty good in both light and dark, but the others are dark enough that they are difficult in dark themes).
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.
Used built-in token colors. They already have dark theme variations, which is nice. I slightly dislike the fact that there no highlighting for strings in the light theme, but this is a very minor issue. We could always go back to this later and tweak some more if we want.
This is a reincarnation of #911
This PR contains:
What is the current behavior?
Samples can be filtered based on simple conditions including one scorer.
What is the new behavior?
Samples can be filtered by compound expressions like
result == "C" and steps <= 10
. Additionally, samples can be filtered based on input and target texts.result == "C"
.Auxiliary changes:
score.foo
vsother_score.foo
.Does this PR introduce a breaking change?
No.
Other information:
Next steps:
This PR is a work in progress. In particular, I remember that @dragonstyle suggested to only apply filter on Enter. This haven't been done yet. I'm also still figuring out some corner cases with different score types. Still, @dragonstyle, if you could take a look at the current state I would appreciate your feedback. Do you this is moving the right direction?