Support searching and sorting experiments via API and sorting on UI #280

Datron · 2024-11-08T09:08:32Z

Problem

It is difficult to find experiments on different filters and sort them

Solution

Provide an API interface that allows a user to search and sort experiments based on multiple parameters.
Add support in the UI to sort columns, and in experiments listing add the ability to sort on last_modified_at and created_at

PS: more frontend changes are to be expected to support searching. That is NOT present in this PR.

Screen.Recording.2024-11-08.at.2.35.02.PM.mov

pratikmishra356 · 2024-11-22T11:04:46Z

crates/superposition_types/src/lib.rs

+
+impl Default for SortBy {
+    fn default() -> Self {
+        Self::Desc


I feel default should be Asc

@pratikmishra356 you always want to see the latest data, especially if you've been using Superposition for years.

@Datron but it can differ based on entity , position/weight needs to be sort in asc order

You can set it in the sort function based on needs, don't need to call default

pratikmishra356 · 2024-11-22T11:07:31Z

crates/experimentation_platform/src/api/experiments/handlers.rs

+        }
+        if let Some(ref context_search) = filters.context {
+            builder = builder.filter(
+                sql::<Bool>("context::text LIKE ")


are we type casting here ? context

Yes we are, this just lets me modify the context type so I can use LIKE on it. It's currently an experiment to see if this is a viable way to search through contexts

pratikmishra356 · 2024-11-25T07:30:35Z

crates/context_aware_config/src/api/context/handlers.rs

+                all_contexts = all_contexts
+                    .into_iter()
+                    .filter_map(|mut context| {
+                        Context::filter_keys_by_prefix(&context, &prefix_list)


@Datron filter_keys_by_prefix uses try_from which results in lots of error logs
2024-11-25T07:06:27Z ERROR superposition_types::config] Override validation error: Override is empty
Should we instead check if map is empty and then probably try_from

@pratikmishra356 yes we can, but that is out of scope for this PR

pratikmishra356 · 2024-11-25T08:13:20Z

crates/context_aware_config/src/api/context/handlers.rs

@@ -633,56 +633,54 @@ async fn list_contexts(
    }


should we separate Asc, Desc like you did in experiment/list ?

What would be the benefit? Could you share an example?

@Datron #288 (comment)

ayushjain17 · 2024-11-26T13:26:48Z

crates/superposition_types/src/custom_query.rs

+#[derive(Debug, Deserialize, Clone, Deref)]
+#[deref(forward)]
+pub struct StringArgs(
+    #[serde(deserialize_with = "deserialize_stringified_list")] pub Vec<String>,
+);
+
+pub fn deserialize_stringified_list<'de, D, I>(


can we instead implement the Deserialize trait on StringArgs and write the logic in it instead of writing this as a function

I tried Deserialize, it was more verbose, and all examples I found did it this way. Can we continue with this method for now?

let me know if this works:
https://github.com/juspay/superposition/pull/280/files#r1865864524

docker-compose/postgres/Dockerfile

crates/experimentation_platform/src/api/experiments/types.rs

ayushjain17 · 2024-11-26T13:28:58Z

crates/experimentation_platform/src/api/experiments/handlers.rs

+        }
+        if let Some(ref context_search) = filters.context {
+            builder = builder.filter(
+                sql::<Bool>("context::text LIKE ")


what is this query doing here ?

crates/frontend/src/components/table.rs

ayushjain17 · 2024-11-26T13:31:35Z

crates/frontend/src/types.rs

@@ -122,10 +126,71 @@ pub struct ExperimentsResponse {
 pub struct StatusTypes(pub Vec<ExperimentStatusType>);

 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
-pub struct ExpListFilters {
+pub struct ExperimentListFilters {


is this type not there in experimentation_platform for the api type ?

It is present, do you recommend moving this to superposition_types?

its good to have, or we can wait till this PR goes in:
#291

after this PR is merged, I would raise a separate PR for moving the api types to superposition_types probably we can move it then

crates/frontend/src/pages/experiment_list/utils.rs

please re-review

crates/experimentation_platform/src/api/experiments/handlers.rs

ayushjain17 · 2024-12-02T13:36:00Z

crates/superposition_types/src/custom_query.rs

+#[derive(Debug, Deserialize, Clone, Deref)]
+#[deref(forward)]
+pub struct StringArgs(
+    #[serde(deserialize_with = "deserialize_stringified_list")] pub Vec<String>,
+);
+
+pub fn deserialize_stringified_list<'de, D, I>(
+    deserializer: D,
+) -> std::result::Result<Vec<I>, D::Error>
+where
+    D: de::Deserializer<'de>,
+    I: de::DeserializeOwned,
+{
+    struct StringVecVisitor<I>(std::marker::PhantomData<I>);
+
+    impl<'de, I> de::Visitor<'de> for StringVecVisitor<I>
+    where
+        I: de::DeserializeOwned,
+    {
+        type Value = Vec<I>;
+
+        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+            formatter.write_str(
+                "a string containing comma separated values eg: CREATED,INPROGRESS",
+            )
+        }
+
+        fn visit_str<E>(self, v: &str) -> std::result::Result<Self::Value, E>
+        where
+            E: de::Error,
+        {
+            let mut query_vector = Vec::new();
+            for param in v.split(',') {
+                let p: I = I::deserialize(param.into_deserializer())?;
+                query_vector.push(p);
+            }
+            Ok(query_vector)
+        }
+    }
+
+    deserializer.deserialize_any(StringVecVisitor(std::marker::PhantomData::<I>))
+}
+


this should work, right ?

Suggested change

#[derive(Debug, Deserialize, Clone, Deref)]

#[deref(forward)]

pub struct StringArgs(

#[serde(deserialize_with = "deserialize_stringified_list")] pub Vec<String>,

);

pub fn deserialize_stringified_list<'de, D, I>(

deserializer: D,

) -> std::result::Result<Vec, D::Error>

where

D: de::Deserializer<'de>,

I: de::DeserializeOwned,

{

struct StringVecVisitor(std::marker::PhantomData);

impl<'de, I> de::Visitor<'de> for StringVecVisitor

where

I: de::DeserializeOwned,

{

type Value = Vec;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {

formatter.write_str(

"a string containing comma separated values eg: CREATED,INPROGRESS",

)

}

fn visit_str<E>(self, v: &str) -> std::result::Result<Self::Value, E>

where

E: de::Error,

{

let mut query_vector = Vec::new();

for param in v.split(',') {

let p: I = I::deserialize(param.into_deserializer())?;

query_vector.push(p);

}

Ok(query_vector)

}

}

deserializer.deserialize_any(StringVecVisitor(std::marker::PhantomData::))

}

#[derive(Debug, Clone, Deref)]

#[deref(forward)]

pub struct StringArgs(pub Vec<String>);

impl<'de> Deserialize<'de> for StringArgs {

fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>

where

D: Deserializer<'de>,

{

let s = String::deserialize(deserializer)?;

let items = s.split(',').map(|item| item.trim().to_string()).collect();

Ok(Self(items))

}

}```

In the case you present, if the user passes a wrong value then there is no error, which I want.

this does return an error, if the data passed is not a string, which is more than enough to determine if the value is right or not
as the data in it must contain is not mandatory, because there can always be a single element in the array which does not require any sorts of delimiter

@ayushjain17 I tried this, but this function is used to deserialize a lot of other vectors and type cast them. I don't want to manually impl deserialize on all of them so I won't change this for now.

crates/experimentation_platform/src/api/experiments/types.rs

crates/frontend/src/types.rs

ShubhranshuSanjeev · 2024-12-03T13:50:46Z

crates/superposition_types/src/lib.rs

+)]
+#[serde(rename_all = "lowercase")]
+pub enum SortBy {
+    #[display(fmt = "desc")]


Is a dedicated how to display definition required, rename_all doesn't handle this case too?

rename_all is for deserialization, the to_string representation would be different me thinks

ShubhranshuSanjeev · 2024-12-03T13:54:17Z

crates/frontend/src/pages/experiment_list/utils.rs

+    #[strum(to_string = "last_modified_at")]
+    LastModifiedAt,
+    #[strum(to_string = "created_at")]
+    CreatedAt,


This also won't rename_all handle the case of to_string which is just the snake_case repr.

Didn't get you

he meant why do we need strum to_string here, is serde rename_all not enough here ?
just like here

crates/frontend/src/types.rs

crates/experimentation_platform/src/api/experiments/types.rs

ShubhranshuSanjeev · 2024-12-03T14:25:31Z

crates/frontend/src/components/table/types.rs

+pub enum ColumnSortable {
+    Yes {
+        sort_fn: Callback<()>,
+        sort_by: SortBy,


Suggested change

sort_by: SortBy,

order: Order,

crates/frontend/src/components/table/types.rs

crates/frontend/src/components/table.rs

…created_at

please re-review

ayushjain17 · 2024-12-10T07:27:10Z

crates/frontend/src/api.rs

+            filters.to_string(),
+            pagination


can we make it consistent over here, I got confused that how are these two not having the exact same syntax

Suggested change

filters.to_string(),

pagination

filters.to_string(),

pagination.to_string()

You won't get confused if you see the whole function.

ayushjain17 · 2024-12-10T07:30:54Z

crates/frontend/src/api.rs

+        format!(
+            "{}/experiments?{}&{}",
+            host,
+            filters.to_string(),
+            pagination
+        )


this does not seem correct, if someone does not send filters, the url will become:
base_url/experimnets?&pagi_filt1=abc

There are always date filters

that is not being guaranteed by the type
all fields in ExperimentListFilters are optional, so one can simply avoid sending any params and then it breaks over here

It is guaranteed by code,

from_date: Utc.timestamp_opt(0, 0).single(), to_date: Utc.timestamp_opt(4130561031, 0).single(),

https://github.com/juspay/superposition/pull/280/files/64469a660bb91eabe003b378e3a0ec64d2de3961#diff-a26bd230250ef10be78fd6369216b71e556f5b808a9cd12e2be3b6551c6e8cbbR41

Doesn't need to be guaranteed by the type

ayushjain17 · 2024-12-10T07:47:15Z

crates/frontend/src/components/table.rs

+                                            let value: String = row
+                                                .get(cname)
+                                                .unwrap_or(&Value::String(String::new()))
+                                                .html_display();


can we rather do something like this:

Suggested change

let value: String = row

.get(cname)

.unwrap_or(&Value::String(String::new()))

.html_display();

let value = row.get(cname).map(|v| v.html_display()).unwrap_or_default();

Why the difference?

its better memory wise, right
in the default case we just need a String, and thats what would be get directly

else, currently in default case, you are getting a Value type which again is converted to String by html_display

Will look into that, can we take it as it is now?

Datron requested a review from a team as a code owner November 8, 2024 09:08

Datron requested a review from ShubhranshuSanjeev November 12, 2024 03:20

Datron force-pushed the search-experiments branch 2 times, most recently from c666640 to 523a478 Compare November 18, 2024 06:41

Datron mentioned this pull request Nov 20, 2024

Search experiments UI #288

Open

pratikmishra356 reviewed Nov 22, 2024

View reviewed changes

pratikmishra356 reviewed Nov 25, 2024

View reviewed changes

Datron force-pushed the search-experiments branch 2 times, most recently from 94e1bd4 to 4a57a01 Compare November 26, 2024 10:48

ayushjain17 previously requested changes Nov 26, 2024

View reviewed changes

Datron force-pushed the search-experiments branch from 4a57a01 to 9bc4033 Compare November 29, 2024 10:30

Datron requested review from ayushjain17 and pratikmishra356 November 29, 2024 10:31

Datron force-pushed the search-experiments branch from 9bc4033 to bc964e7 Compare November 29, 2024 10:32

ayushjain17 reviewed Dec 2, 2024

View reviewed changes

crates/experimentation_platform/src/api/experiments/handlers.rs Outdated Show resolved Hide resolved

ayushjain17 reviewed Dec 2, 2024

View reviewed changes

Datron force-pushed the search-experiments branch from bc964e7 to e2ee925 Compare December 3, 2024 12:47

Datron requested a review from ayushjain17 December 3, 2024 12:52

ShubhranshuSanjeev previously requested changes Dec 3, 2024

View reviewed changes

Datron added 2 commits December 9, 2024 14:22

feat: search and sort experiments

80ad64b

feat: sort experiments in the frontend based on last_modified_at and …

64469a6

…created_at

Datron force-pushed the search-experiments branch from e2ee925 to 64469a6 Compare December 9, 2024 11:35

Datron requested a review from ShubhranshuSanjeev December 9, 2024 11:37

ayushjain17 reviewed Dec 10, 2024

View reviewed changes

ShubhranshuSanjeev approved these changes Dec 10, 2024

View reviewed changes

ayushjain17 approved these changes Dec 11, 2024

View reviewed changes

ayushjain17 merged commit 95b87c5 into main Dec 11, 2024
4 checks passed

ayushjain17 deleted the search-experiments branch December 11, 2024 11:39

Support searching and sorting experiments via API and sorting on UI #280

Support searching and sorting experiments via API and sorting on UI #280

Conversation

Datron commented Nov 8, 2024 • edited Loading

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datron commented Nov 8, 2024 •

edited

Loading

ayushjain17 Dec 10, 2024 •

edited

Loading

ayushjain17 Dec 10, 2024 •

edited

Loading