Repository navigation
feat(hardware): filter the hardware page by the registry - #2192
alanpeixinho wants to merge 20 commits into
Conversation
|
| if revision: | ||
| checkout_join = _REVISION_JOIN | ||
| day_filter = "" | ||
| else: | ||
| checkout_join = _CHECKOUT_JOIN if commits else "" | ||
| day_filter = """ | ||
| AND daily.checkout_day >= %(start_day)s | ||
| AND daily.checkout_day <= %(end_day)s | ||
| """ | ||
| return f""" | ||
| {rows_name}_rows AS ( | ||
| SELECT daily.* | ||
| FROM {table} daily | ||
| {checkout_join} | ||
| WHERE daily.{origin_column} = %(origin)s | ||
| {day_filter} | ||
| ), | ||
| {rows_name}_hardware AS ( | ||
| SELECT DISTINCT ON (platform) platform, compatibles | ||
| FROM {rows_name}_rows | ||
| WHERE compatibles IS NOT NULL | ||
| ORDER BY platform, CARDINALITY(compatibles) DESC, compatibles | ||
| ), | ||
| {rows_name}_counts AS ( | ||
| SELECT | ||
| daily.platform, | ||
| hardware.compatibles AS hardware, | ||
| {sums} | ||
| FROM {rows_name}_rows daily | ||
| LEFT JOIN {rows_name}_hardware hardware USING (platform) | ||
| GROUP BY daily.platform, hardware.compatibles | ||
| ) | ||
| """ | ||
|
|
||
|
|
||
| def _daily_listing_query(*, commits: bool, revision: bool, where: str) -> str: | ||
| return f""" | ||
| WITH { | ||
| _side_counts( | ||
| rows_name="build", | ||
| table="hardware_daily_builds", | ||
| origin_column="build_origin", | ||
| sums=''' | ||
| SUM(daily.build_pass) AS build_pass, | ||
| SUM(daily.build_failed) AS build_failed, | ||
| SUM(daily.build_inc) AS build_inc | ||
| ''', | ||
| commits=commits, | ||
| revision=revision, | ||
| ) | ||
| }, |
There was a problem hiding this comment.
our UI says that we are showing data only to the latest checkouts but this PR sums all daily rows. Is that correct?
There was a problem hiding this comment.
Fixed in 4cd3170. The footer stays. The window listing now keeps the latest checkout of each tree for each platform this origin tested, which is what that text describes. The filter options use those same checkouts.
| search: previousSearch => ({ | ||
| ...previousSearch, | ||
| registryFilter: cleanRegistryFilter(draft), | ||
| }), | ||
| state: s => s, | ||
| }); | ||
| }, [draft, navigate]); | ||
|
|
||
| const sections = useMemo( | ||
| () => | ||
| REGISTRY_FILTER_FIELDS.flatMap(field => { | ||
| const choices = optionsQuery.data?.[field.optionKey] ?? []; | ||
| if (choices.length === 0) { | ||
| return []; | ||
| } | ||
| const selected = new Set(draft[field.key] ?? []); | ||
| return [ | ||
| { | ||
| key: field.key, | ||
| title: intl.formatMessage({ id: field.label }), | ||
| items: Object.fromEntries( | ||
| choices.map(choice => [choice, selected.has(choice)]), | ||
| ), |
There was a problem hiding this comment.
we are omitting selected values absent from current options but keeping them in state, leaving no UI control to remove them.
There was a problem hiding this comment.
Fixed in 4cd3170. A selected value that is no longer an option stays in the list, so it can be unchecked.
| _side_counts( | ||
| rows_name="build", | ||
| table="hardware_daily_builds", | ||
| origin_column="build_origin", |
There was a problem hiding this comment.
We filter builds by build_origin. hardware_status stores those counts on the test_origin row. A PASS maestro build with a ti boot returns build_pass=1 for origin=ti today. This query returns build_pass=0. The same build is counted under origin=maestro with no boot.
There was a problem hiding this comment.
Fixed in 4cd3170. The listing no longer filters hardware_daily_builds by build_origin. A build row is kept when a test of this origin shares its checkout and platform. Origin ti now returns build_pass for that maestro build, together with the ti boot. Origin maestro counts it only when maestro tested it.
There was a problem hiding this comment.
The EXISTS check fixes the ti example. It still counts every build on that checkout and platform once any test of this origin exists there.
Take checkout C on platform P with builds B1 and B2. Maestro tests B1. Ti tests B2. Both origin=ti and origin=maestro now report two builds. hardware_status reports one each. The comment above this EXISTS says both build origins are included.
hardware_daily_builds has no test origin. INSERT_BUILDS groups by checkout, build_origin, lab, and platform, and drops t.origin from day_tests.
Add a test_origin column to hardware_daily_builds and include it in the primary key. In INSERT_BUILDS, keep t.origin through the DISTINCT and the GROUP BY, and store it as test_origin. Replace this EXISTS with daily.test_origin = %(origin)s, the same filter the tests side uses.
Add an integration test with those two builds and assert that each origin's build count is 1.
There was a problem hiding this comment.
Fixed in 16ee4f2. The listing filters hardware_daily_builds on test_origin. Maestro testing B1 and ti testing B2 now reports 1 build each. The integration test asserts that.
|
merge conflicts |
felipebergamin
left a comment
felipebergamin
left a comment
There was a problem hiding this comment.
Clear does not remove all selected trees on hardware details.
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| // ponytail: one toggle per value. Callers apply functional updates, so they chain. One onClear if a caller cannot. | ||
| selected.forEach(onClickItem); |
There was a problem hiding this comment.
Use a functional state update in HardwareDetailsFilter.handleSelectTree, or clear all selections in one update.
Reproduced in the browser: with 10 trees selected, clicking Clear leaves 9 selected. selected.forEach(onClickItem) calls the handler repeatedly, but each call reads the same treeIndexes, so only the last removal takes effect. Add a test that clears multiple selected trees.
There was a problem hiding this comment.
Fixed in 16ee4f2. handleSelectTree updates from the previous state, so Clear removes every selected tree.
4cd3170 to
5e67dca
Compare
|
[P1] Backfill Migration 0027 assigns I ran the revision SQL on PostgreSQL with a CTE fixture representing an existing row after this migration: 7 passing builds with The cron rebuilds only the last 15 checkout days. Older retained aggregates remain invisible, including pinned revisions. The startup migration script does not backfill them.
Reviewed at |
|
Backfill Since the last review, this PR added only The 13 focused query/view tests pass. The new browser test covers clearing selected trees; it does not cover this upgrade. Implement the backfill/rollout and historical-data handling requested in the previous review, with an upgrade test using populated aggregates older than the 15-day cron window. |
What you think would be the bets approach here? Create a separate issue for the backfill? Include a fallback going to raw fact tables (slower) for non backfilled? other alternative? |
I would avoid fallbacks to slow raw queries. My main concern is the old aggregates whose raw data has already been pruned and we can't recompute. With the current implementation we would exclude them from counts. Maybe all we can do is accept that we can't recompute those older registries |
|
I'm approving like we discussed earlier today |
48ef867 to
7e49b3b
Compare
|
I'm not sure about this one. The page still calls _hardware_daily_counts. It sums every day in the window and filters builds by build_origin, so test_origin is never checked. get_hardware_daily_listing picks the latest checkout and filters by test_origin, but apparently only the tests call it. |
felipebergamin
left a comment
felipebergamin
left a comment
There was a problem hiding this comment.
Comments are on the changed lines.
| INSERT INTO hardware_daily_builds ( | ||
| checkout_day, checkout_id, checkout_origin, build_origin, build_lab, platform, | ||
| compatibles, build_pass, build_failed, build_inc | ||
| checkout_day, checkout_id, checkout_origin, test_origin, build_origin, build_lab, |
There was a problem hiding this comment.
_hardware_daily_counts sums hardware_daily_builds and does not filter or group by test_origin. After this insert, a build tested by two origins on the same platform is stored twice and counted twice on /hardware and the revision listing. Dedupe in that query, or do not key build rows by test origin.
There was a problem hiding this comment.
You are right, and the column is gone. Keying build rows by test origin made _hardware_daily_counts double count, so I dropped the test_origin column, migration 0027, and the recompute change. recompute_hardware_daily.py, update_db.py, and models.py are back to main in 75a5769.
The listing is byte-identical to main again. Without a registry filter both endpoints run the same SQL with the same parameters, and with one they only add a WHERE on the platform. The build origin question from your earlier review is a real bug, but it predates this PR and belongs in its own change rather than inside a filter feature.
There was a problem hiding this comment.
Closing the footer question from your first review, and the answer turned out to be the opposite of what I assumed.
I had been treating the window sum as a regression. It is not. The text is from #1955, and #1990 then deliberately stopped limiting the hardware listing to the latest checkout, so a board tested on an older checkout in the window stays visible. That is what #1983 asked for, and the commit says so: "Non longer limiting hardware listing to latest checkout." It did not touch messages/index.ts, so the text has been stale since 9 September.
So the text changes, not the query. Narrowing the listing to one checkout per tree would reintroduce the bug #1983 was filed to remove, which is also why reverting the test_origin work was the right call rather than a retreat.
The hardware listing now reads "Showing every checkout for trees updated in the last N days". The tree listing joins latest_checkout, so it keeps the original text.
I also renamed the second drawer trigger. Both were "Filters", which is ambiguous for a screen reader and made getByRole('button', { name: 'Filters', exact: true }) match two elements, breaking three tests in hardware-listing.spec.ts. The registry one is now "Registry filters" and those tests pass.
Two tests in that spec still fail, loads hardware listing page correctly and clearing the checkout origin keeps every origin in the URL. I checked them against a worktree on upstream/main and they fail identically there, so they are pre-existing and not from this branch. Playwright does not run in CI, which is presumably why they went unnoticed. Happy to open a separate issue.
| return f"WHERE {predicate}", params | ||
|
|
||
|
|
||
| def get_hardware_daily_listing( |
There was a problem hiding this comment.
HardwareView and HardwareByRevisionView call get_hardware_listing_data and get_hardware_listing_data_by_revision. Nothing outside tests calls get_hardware_daily_listing or get_hardware_daily_revision_listing. Delete these two functions and point the tests at the functions the views call, or switch the views to these functions.
There was a problem hiding this comment.
Deleted. get_hardware_daily_listing and get_hardware_daily_revision_listing are gone and the tests point at get_hardware_listing_data and get_hardware_listing_data_by_revision, the functions the views call. That cut hardwareRegistryFilters.py from 424 lines to about 155.
| origin=request.GET.get("origin") or DEFAULT_ORIGIN, | ||
| start_date=query_params.start_date, | ||
| end_date=query_params.end_date, | ||
| tree_name=request.GET.get("tree_name"), |
There was a problem hiding this comment.
These options use the root origin param and, in get_hardware_registry_filter_options, the latest checkout of each tree. The listing uses the diffFilter origins, every checkout in the window, and commits_list. With a test origin of ti, or with commits selected, the options are not the platforms in the listing. Build the options from the same parameters as the listing.
There was a problem hiding this comment.
Fixed in 75a5769. The view now calls the listing queries themselves, through HardwareQueryParams.from_request or HardwareListingByRevisionQueryParams.from_request, and reads the registry for the platforms they return. It takes the same diffFilter origins, labs, commitsList, and revision keys as the table, so a test origin of ti or a pinned commit gives options for exactly the boards listed. Covered by a view test per path and an integration test that asserts the option lists.
| migrations.RunSQL( | ||
| sql=""" | ||
| ALTER TABLE hardware_daily_builds | ||
| ADD COLUMN test_origin varchar(100) NOT NULL DEFAULT ''; |
There was a problem hiding this comment.
Existing rows stay at test_origin '' until someone runs recompute_hardware_daily for past days, and the PR does not say to do that. The reverse SQL recreates the old primary key, which fails once two origins have rows for the same build. Document the recompute in the deploy notes, and make the migration irreversible or collapse the duplicates in the reverse.
There was a problem hiding this comment.
Dropped the migration entirely, along with the column it added, for the double counting you flagged in recompute_hardware_daily.py. No migration, model, or recompute change is left in this PR, so there is nothing to document in the deploy notes and no reverse SQL to make irreversible.
| build_pass=record[7] or 0, | ||
| build_failed=record[8] or 0, | ||
| build_inc=record[9] or 0, | ||
| test_origin=record[3], |
There was a problem hiding this comment.
Restore reads fields by position. A snapshot taken before this column has 10 fields, so record[3] is build_origin and every later field moves. Check that the row has 11 fields before inserting, and reject a shorter row.
There was a problem hiding this comment.
Reverted. update_db.py is back to main in 75a5769, so restore still reads 10 fields and a snapshot taken before this PR loads unchanged. The positional read is a real fragility, but with the column gone there is no version skew to guard against here.
| setOpen(current => !current); | ||
| } | ||
| }} | ||
| role="combobox" |
There was a problem hiding this comment.
This trigger is a div with role="combobox" and it contains button elements. It has no aria-controls, no aria-labelledby, and no Escape handler. The list below already uses Command. Wrap the trigger with Popover from ui/popover.tsx, which handles focus and Escape.
There was a problem hiding this comment.
Fixed in 73fe8f5, and you were right about the cause as well as the symptom. The trigger is now a real button wrapped in Popover, and the chips and Clear sit beside it rather than inside it, so it holds no interactive children. Radix supplies aria-controls, aria-expanded, and aria-haspopup, and the section title names it through aria-label.
Verified in the browser on the registry drawer:
- trigger is
BUTTONwitharia-controls=radix-«r49»,aria-haspopup=dialog, and 0 nested buttons - Enter opens it and focus lands in the search input
- Escape closes only the list, the drawer stays open, and focus returns to the trigger
- outside click still dismisses, and a value can be picked with the keyboard alone
Escape closing the whole drawer is exactly what I hit while testing the registry filters, so thanks for pointing at the right fix.
* Filter the listing and the revision listing by architecture, processor, platform vendor, silicon vendor, platform, system module, form factor, and board type * Treat values in one field as OR and different fields as AND * Match a board that is missing from the registry only for a lone platform id * Read hardware_daily_builds and hardware_daily_tests, and offer dropdown values from that same window or checkout * Keep the selection in the URL and document the parameters in the schema Part of kernelci#2166 Assisted-by: Cursor Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
* Show the hardware registry filters in the shared filters drawer * Use compact checkbox chips, and a searchable multi-select once a section has more than 8 options * Clear a multi-select with one control * Drop the repeated select prompts and put duration min and max on one line Part of kernelci#2161 Part of kernelci#2166 Assisted-by: Cursor Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Keep the latest checkout of each tree for each platform, count a build on the origin that tested it, and keep a dropped filter value in the drawer. Part of kernelci#2166 Assisted-by: Cursor Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
* Store test_origin on hardware_daily_builds and filter the listing on it * Build the new primary key concurrently, then attach it * Keep that origin through the daily rebuild and the update_db snapshot * Count a build only for the origin that tested it * Clear every selected tree in the hardware details filter Part of kernelci#2166 Assisted-by: Cursor Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Spectacular sorts the registry sync path after the registry filters, so the pre-commit schema check failed. Part of kernelci#2166 Assisted-by: Cursor Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Clear toggles each selected tree in one tick. Two or more trees stay selected if the handler reads stale state. Part of kernelci#2166 Assisted-by: Cursor Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Keep the origin filter and the registry filter together in the header. Part of kernelci#2166 Assisted-by: Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
The listing test still patched the old daily query. The options endpoint read origin off listing params that no longer have that field, and the hardware filter sections still carry subtitles the tightened drawer had dropped. Part of kernelci#2166 Assisted-by: Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
The revision listing test still expected the query without registry_filter. schema.yml also had the old parameter order, and the registry options origin was documented as the checkout origin. Part of kernelci#2166 Assisted-by: Grok 4.7 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Drop test_origin from hardware_daily_builds. The live listing sums builds by build_origin, so splitting a build per test origin counted it once for each origin that tested it. Part of kernelci#2166 Assisted-by: Cursor Claude Opus 5.5 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Keep the hardware listing as on main and take registry options from its rows. The options endpoint now takes the listing filters, runs the listing without the registry filter, and reads the registry for the platforms it returns. Remove the latest-checkout listing that only the tests called. Part of kernelci#2166 Assisted-by: Cursor Claude Opus 5.5 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Part of kernelci#2166 Assisted-by: Cursor Claude Opus 5.5 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Run the registry filter and its options against the database. Part of kernelci#2166 Assisted-by: Cursor Claude Opus 5.5 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Daily rows store `vendor,board` while the registry stores `board`, so the join never matched. Strip the vendor prefix before looking a platform up. A platform id holds a comma, so the comma-separated filter values split one id into two. Carry the registry fields as repeated query parameters, and escape the separator in the URL. Assisted-by: Claude Opus 5 (Cursor) Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Assisted-by: Claude Opus 5 (Cursor) Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
7979fa3 to
a028a9c
Compare
|
Pushed fixups for two bugs this branch had since its first commit. Both come from the same thing: a daily platform id is The registry join never matched. Of 163 boards listed over 5 days, 0 matched a registry id directly. The architecture, processor, vendor, system module, form factor, and board type fields returned nothing, and the drawer offered no options for them. The lookup now strips the vendor prefix, which matches 17 registry ids over 1270 daily rows in the same window. A platform id holding a comma could not be filtered. It split into two values in two places: the query string, where the fields were comma-separated, and the URL, where Tests that fail without the fixes: Checked against the production database over a 5 day window:
Every one of those also works as a pasted link, not just as a click in the drawer. Registry options now populated, architecture filter applied, and two comma-bearing platform ids filtered from a pasted link:
|
The trigger was a div with `role="combobox"` wrapping buttons, with no `aria-controls`, no name, and no Escape handler. Escape closed the whole filter drawer instead of the list. Use `Popover`, and make the trigger a real button with the chips and Clear beside it so it holds no interactive children. Assisted-by: Claude Opus 5 (Cursor) Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
`replaceAll` needs es2021, and `tsc -b` targets lower. Assisted-by: Claude Opus 5 (Cursor) Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
`CheckboxChips` takes no title. Assisted-by: Claude Opus 5 (Cursor) Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
The footer has said "latest checkout" since kernelci#1955, but kernelci#1990 stopped limiting the hardware listing to the latest checkout so boards tested on older checkouts in the window stay visible. The counts have covered the whole window ever since. The tree listing still joins latest_checkout, so it keeps the old text. Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi> Assisted-by: Claude Opus 5 (Cursor) Part of kernelci#2166 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Two triggers both named "Filters" are ambiguous for a screen reader, and they broke the e2e helper that looks the button up by name. Assisted-by: Claude Opus 5 (Cursor) Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
What it is
Part of #1886. Closes #2166. Part of #2161.
The hardware page can filter by the shared hardware registry. The fields are architecture, processor, platform vendor, silicon vendor, platform, system module, form factor, and board type. They open from a second drawer, "Registry filters", and the selection stays in the URL.
Values in one field are OR. Different fields are AND. A board missing from the registry still appears. It matches only a platform id, and only when that is the only field set.
What does not change
The listing counts are the same as on
main. Without a registry filter,/api/hardware/and/api/hardware-by-revision/run the same SQL with the same parameters. With one, the query only adds aWHEREon the platform.The registry options come from the listing itself.
/api/hardware/registry-filters/takes the same origin, lab, commit, and revision parameters as the listing, runs it without the registry filter, and reads the registry for the platforms it returns. The drawer offers the boards the table shows, and picking one field does not hide the choices in another.No migration, model, or daily recompute change.
Shared filter drawer
This also changes the drawer used on tree details, hardware details, issue listing, and the hardware origin filters:
Popoverwhose trigger is a button, with the chips and Clear beside it so the trigger holds no interactive children. Escape closes the list and leaves the drawer open, and focus returns to the trigger. Shorter sections stay checkbox chips on purpose, since every option is already visible and a search adds a step. The registry platform list needed this, and long lists elsewhere (configs, hardware, labs) get it too.handleSelectTreenow uses a functional update.hardware-details.spec.tscovers it.OptionFiltersnow show their display text. Onmainthe lookup returned inside aforEach, so the raw value always showed.Platform ids carry a vendor prefix
Daily rows store the platform as
ti,am335x-bone-black. The registry stores the board alone,am335x-bone-black. Two things follow:stringifySearchescapes the separator inside a value so the URL round-trips.The footer said the wrong thing
The hardware table footer said "Showing latest checkout for trees updated in the last N days". That text is from #1955, and #1990 then stopped limiting the hardware listing to the latest checkout so boards tested on an older checkout in the window stay visible, which is what #1983 asked for. The counts have covered the whole window ever since, and nobody updated the text.
So the text changes, not the query. Narrowing the listing to one checkout per tree is the behavior #1983 was filed to remove. The hardware listing now reads "Showing every checkout for trees updated in the last N days". The tree listing still joins
latest_checkout, so it keeps the original text.The two drawer triggers were both named "Filters". The registry one is now "Registry filters", since two controls with the same name are ambiguous for a screen reader and broke the e2e helper that finds the button by name.
How to test
/hardware, the footer reads "Showing every checkout...". On/treeit still reads "Showing latest checkout..."./hardware, open Registry filters and selectarm64. The list narrows to arm64 boards, and a reload keeps the selection.ti,am335x-bone-black. One board is listed, and the shared link restores it.mainfor the same window and origin filters.Tests:
hardwareRegistryFilters_test.py(predicate, options query, options view),hardware_listing_test.py(registry filter and options on Postgres, with a vendor-prefixed platform),registryFilters.test.ts,search.test.ts(URL round-trip of an id holding the separator),hardware-details.spec.ts.