Refactor system table filter and sorter #518

qjiang002 · 2022-11-16T21:38:08Z

This PR is related to issue #485

Previous filters and sorters

Refactor: move filter lists and sorter to the system table header

It passes the filter and sorter settings to the backend client and retrieve a new list of systems every time.
It supports one sorted column at a time as before. The default sorted column is Created At in descending order.
It can also update and parse the settings in the url.

qjiang002 · 2022-11-17T18:47:13Z

New commit: support search by system and dataset name with a single search bar.

pfliu-nlp · 2022-11-18T18:34:26Z

hi, @qjiang002 Cool, thanks for such a fast implementation.

"support search by system and dataset name with a single search bar."

I wonder whether it could work when users just type a system name, such as bert or they must type system:bert?
I'm not sure if the latter one is convenient enough.

qjiang002 · 2022-11-18T20:02:11Z

Now it requires the prefix "system: bert" to search systems. One way to simplify this is to add a dropdown list before the search bar to auto-fill the prefix. WDYT

neubig · 2022-11-18T22:56:56Z

@qjiang002 : what about if it doesn't start with "system:" or "dataset:" then we allow it to search any field. Would that work?

lyuyangh

Thanks, @qjiang002 ! I think we can definitely make the filtering UI better and this is in the right direction!

I think the reason why we wanted to change the design is that the search bar has become too crowded. Moving the task filter and dataset split filter to the table seems to be a good strategy and they are quite intuitive to use.

[dataset/system filter] I think it'll be more intuitive to use if we have separate input boxes for them. Or, if we want to save more space, could we move the dataset filter to the table header as well (It can be an input box instead of a list of options to select)? I guess the system filter may be the most used (?) so we can leave it in the search bar.
- I think we should provide options (or autocomplete) for dataset filtering. i.e. GET /systems?dataset= cnn_dailymail only supports filtering by the full dataset name. The client can get a list of datasets for autocompletion by querying GET /datasets?name=cnn
- Regarding @neubig's suggestion, I think it is doable but could we create a separate PR for that? We need to change the backend and the "URL linking" logic for the frontend to make it work.
  - We still need to support filtering by dataset only for the leaderboard to work. We can add another filter called dataset_or_system for GET /systems.
  - To make it obvious why the records show up in the results, I think we should highlight the cells that match the query.
    - May not be a good example, but if a user wants to see all systems for "cnn_dailymail" and they typed in "cnn". They would get a bunch of systems that have "cnn" (the model) in their names. This may be confusing.
[Sorting] We have many columns (the table is very wide) so I am not sure if putting the sorter in the table header is the best option. Also, when we are sorting, the order of columns may change. If we want to put it in the header, I think we should make it obvious which column is currently being used to sort the records. We can make that column always visible (fixed to the right or left) and/or highlight it. And all the other columns should not change order.

lyuyangh · 2022-11-18T20:30:35Z

backend/src/impl/db_utils/system_db_utils.py

@@ -135,11 +135,13 @@ def find_systems(
        if ids:
            search_conditions.append({"_id": {"$in": [ObjectId(_id) for _id in ids]}})
        if system_name:
-            search_conditions.append({"system_name": {"$regex": rf"^{system_name}.*"}})
+            search_conditions.append({"system_name": {"$regex": rf"{system_name}.*"}})


I think these would require scanning the entire database which may not scale. Could you please confirm?

lyuyangh · 2022-11-19T14:23:03Z

frontend/src/components/SystemsTable/SystemTableContent.tsx

@@ -220,6 +247,50 @@ export function SystemTableContent({
    },
  };

+  const handleTableChange = (


Instead of implementing all the table change logic in one function, I think it'll be more readable if it is implemented for each column where the filter is defined so you don't have to write these if-else statements to match filter names. There does not seem to be an onSort() so we probably have to write sorter logic in this function though. WDYT?

@lyuyangh, I try to use the onFilter property in ColumnsType definition to call the onFilterChange function to update the filtered system table, but I met some problems.

onFilter: (value, record) => boolean: reseting the filter doesn't set the value to be null. onFilter is not called.

<Table onChange={onChange} /> must have a onChange function so that the column's onFilter can be called.

Therefore, in PR #528 , I still keep the handleTableChange function since Table's onChange property must have value and filters can get the reset null value directly.

lyuyangh · 2022-11-19T14:40:20Z

frontend/src/components/SystemsTable/SystemTableContent.tsx

@@ -158,6 +184,7 @@ export function SystemTableContent({
      render: (_, record) => record.created_at.format("MM/DD/YYYY HH:mm"),
      width: 130,
      align: "center",
+      sorter: true,


I think the sorters also need to be controlled by filterValue similar to the filters. This would allow the table to react to URL changes and show the default sort field.

lyuyangh · 2022-11-19T15:16:44Z

frontend/src/components/SystemsTable/SystemTableContent.tsx

+    }
+    // Handle sorter change
+    // Only one sorted column allowed now
+    if (!(sorter instanceof Array)) {


It took me a while to understand how sort columns and directions are determined. I think something like this would be easier to read:

if (!(sorter instanceof Array)) { if (sorter.column?.title == null || sorter.order == null) { // user cleared sorter, set to default // if not currently using the default sorter onFilterChange({ sortField: "created_at", sortDir: "desc" }); } else { // user modified sorter, apply sorter to filterValue let sortFieldName = sorter.column.title; if (sortFieldName === "Created At") sortFieldName = "created_at"; if (typeof sortFieldName !== "string") { console.error(`sortFieldName=${sortFieldName} is not a string`); } else { onFilterChange({ sortField: sortFieldName, sortDir: sorter.order === "descend" ? "desc" : "asc", }); } } }

I put the default sorter before the more complex sorter logic. The if statement also handles null checks so we don't have to do it in the else block. I think you added toString() to make tsc happy because column.title is a union of many types. Using toString() forces the value to be a string which may hide bugs though so I think using a type check is the safer thing to do.

lyuyangh · 2022-11-19T15:21:27Z

frontend/src/components/SystemsTable/SystemTableTools.tsx

@@ -195,6 +190,24 @@ export function SystemTableTools({
    );
  }

+  const handleSearch = (query: string) => {


I think this sends out a request even if the query is incorrect. Also, it sends requests for each character when the user is entering the first part of the query (e.g. "dataset:"). This will increase the load of the backend and it also makes the frontend less responsive because the requests are in sequence.

lyuyangh · 2022-11-19T16:26:27Z

frontend/src/components/SystemsTable/SystemTableContent.tsx

+        filters[k]?.toString() !== filterValue.split
+      ) {
+        onFilterChange({ split: filters[k]?.toString() });
+      } else if (k === "task" && filters[k]?.toString() !== filterValue.task) {


Looks like filters[k] can sometimes be an array so this does not work properly. I would suggest to avoid toString() as a type cast.

Now the filters only allow one filtered value, so filters[k] is of type FilterValue | null. FilterValue is defined as React.Key | boolean, but the split is of type string | undefined.

In PR #528 , I try to separate the null/undefined and Key/string cases, but I don't find a way to avoid type casting.

qjiang002 · 2022-11-20T17:29:37Z

Thanks @lyuyangh for the detailed suggestions! Here are some of my thoughts to improve the filter/sort function.

[system/dataset]

I agree that it can be clearer to use separate search bar for systems and datasets. For systems, we can keep the search bar above the table as before. For datasets, we can put a searchable list similar to the language dropdown list at the dataset table header.
As for @neubig 's suggestion of searching for system and dataset together, I think users usually want to compare systems using the same methods or using the same datasets, which can be handled by searching by system/dataset/tag separately. Is there any use case that users need to search them together?
Highlight the filter columns if they are filtered.

[sorting]

The metric columns in the table depends on what systems are displayed in the current page, so some metrics are not visible / sortable if there is a system using this metric on the current page. One solution is to use a separate sorting input above the table as before, which will make the filter section longer. If we keep the sorting in each column, we may need to display all the metric columns all the time, which will make the table longer. I would prefer the first option because I find it harder to look for a metric name in the column headers.
We should pin and highlight the sorted table column.

neubig · 2022-11-20T17:48:16Z

As for @neubig 's suggestion of searching for system and dataset together, I think users usually want to compare systems using the same methods or using the same datasets, which can be handled by searching by system/dataset/tag separately. Is there any use case that users need to search them together?

I was just thinking that it's easier to type "sst2" than it is to type "dataset:sst2". If we have separate search bars I think that's not really an issue.

lyuyangh · 2022-11-21T16:48:10Z

@qjiang002 Sounds good! Let's have separate search bars for the dataset and system names. And yeah, I think we can put the sort column selector in the search bar for now. That seems easier to do and it also provides a better user experience. If you want, feel free to split these into multiple PRs.

qjiang002 · 2022-11-22T20:36:25Z

This PR is closed as it is split into separate PRs #528, #529, #530.

qjiang002 and others added 5 commits November 16, 2022 01:47

move task and split filter to table header

c07f0cc

move task and split filter to table header

6e2ced9

move sorting to table header

5bee0b1

move sorting to table header

b4947b2

add inline comment

2914cda

qjiang002 requested a review from pfliu-nlp November 16, 2022 21:38

qjiang002 requested review from neubig and lyuyangh as code owners November 16, 2022 21:38

search by system and dataset name

2a5817b

lyuyangh requested changes Nov 19, 2022

View reviewed changes

This was referenced Nov 21, 2022

move dataset_split and task filter to system table header #528

Merged

search systems by dataset name #529

Merged

fix sorted column of system table #530

Merged

qjiang002 closed this Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor system table filter and sorter #518

Refactor system table filter and sorter #518

qjiang002 commented Nov 16, 2022

qjiang002 commented Nov 17, 2022

pfliu-nlp commented Nov 18, 2022 •

edited

Loading

qjiang002 commented Nov 18, 2022

neubig commented Nov 18, 2022

lyuyangh left a comment

lyuyangh Nov 18, 2022

lyuyangh Nov 19, 2022

qjiang002 Nov 21, 2022

lyuyangh Nov 19, 2022

lyuyangh Nov 19, 2022 •

edited

Loading

lyuyangh Nov 19, 2022 •

edited

Loading

lyuyangh Nov 19, 2022

qjiang002 Nov 21, 2022

qjiang002 commented Nov 20, 2022

neubig commented Nov 20, 2022

lyuyangh commented Nov 21, 2022

qjiang002 commented Nov 22, 2022

Refactor system table filter and sorter #518

Refactor system table filter and sorter #518

Conversation

qjiang002 commented Nov 16, 2022

Previous filters and sorters

Refactor: move filter lists and sorter to the system table header

qjiang002 commented Nov 17, 2022

pfliu-nlp commented Nov 18, 2022 • edited Loading

qjiang002 commented Nov 18, 2022

neubig commented Nov 18, 2022

lyuyangh left a comment

Choose a reason for hiding this comment

lyuyangh Nov 18, 2022

Choose a reason for hiding this comment

lyuyangh Nov 19, 2022

Choose a reason for hiding this comment

qjiang002 Nov 21, 2022

Choose a reason for hiding this comment

lyuyangh Nov 19, 2022

Choose a reason for hiding this comment

lyuyangh Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

lyuyangh Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

lyuyangh Nov 19, 2022

Choose a reason for hiding this comment

qjiang002 Nov 21, 2022

Choose a reason for hiding this comment

qjiang002 commented Nov 20, 2022

neubig commented Nov 20, 2022

lyuyangh commented Nov 21, 2022

qjiang002 commented Nov 22, 2022

pfliu-nlp commented Nov 18, 2022 •

edited

Loading

lyuyangh Nov 19, 2022 •

edited

Loading

lyuyangh Nov 19, 2022 •

edited

Loading