Fix issue where by default a long media doesnt take up the full screen (

#1159) https://lilacai-nikhil-staging.hf.space/datasets#local/mosaic-chat-v2&rowId=%22000006e20ceb477f889e297c22f4e2f2%22 <img width="650" alt="image" src="https://github.com/lilacai/lilac/assets/1100749/4c75b50e-e5ab-44fc-88b9-6d4fc6aa085f">
databricks · Feb 2, 2024 · 0c98360 · 0c98360
1 parent 2df9fd5
commit 0c98360
Show file tree

Hide file tree

Showing 13 changed files with 131 additions and 63 deletions.
diff --git a/lilac/embeddings/openai.py b/lilac/embeddings/openai.py
@@ -44,12 +44,14 @@ def setup(self) -> None:
     azure_api_endpoint = env('AZURE_OPENAI_ENDPOINT')
 
     if not api_key and not azure_api_key:
-      raise ValueError('`OPENAI_API_KEY` or `AZURE_OPENAI_KEY` '
-                       'environment variables not set, please set one.')
+      raise ValueError(
+        '`OPENAI_API_KEY` or `AZURE_OPENAI_KEY` ' 'environment variables not set, please set one.'
+      )
     if api_key and azure_api_key:
       raise ValueError(
         'Both `OPENAI_API_KEY` and `AZURE_OPENAI_KEY` '
-        'environment variables are set, please set only one.')
+        'environment variables are set, please set only one.'
+      )
 
     try:
       import openai
@@ -61,16 +63,13 @@ def setup(self) -> None:
       )
 
     else:
-
       if api_key:
         self._client = openai.OpenAI(api_key=api_key)
         self._azure = False
 
       elif azure_api_key:
         self._client = openai.AzureOpenAI(
-          api_key=azure_api_key,
-          api_version=azure_api_version,
-          azure_endpoint=azure_api_endpoint
+          api_key=azure_api_key, api_version=azure_api_version, azure_endpoint=azure_api_endpoint
         )
         self._azure = True
         OpenAIEmbedding.local_batch_size = AZURE_OPENAI_BATCH_SIZE

diff --git a/lilac/project_test.py b/lilac/project_test.py
@@ -21,7 +21,7 @@ async def test_create_project_and_set_env(tmp_path_factory: pytest.TempPathFacto
 
 
 async def test_create_project_and_set_env_from_env(
-  tmp_path_factory: pytest.TempPathFactory
+  tmp_path_factory: pytest.TempPathFactory,
 ) -> None:
   tmp_path = str(tmp_path_factory.mktemp('test_project'))
 

diff --git a/web/blueprint/src/lib/components/Page.svelte b/web/blueprint/src/lib/components/Page.svelte
@@ -39,7 +39,7 @@
       on:click={() => ($navStore.open = true)}><SidePanelOpen /></button
     >
     <div
-      class="flex flex-grow flex-row items-center justify-between justify-items-center gap-x-6 py-2 pr-12"
+      class="flex flex-grow flex-row items-center justify-between justify-items-center gap-x-6 py-2 pr-8"
     >
       <div class="flex flex-row items-center">
         <div>

diff --git a/web/blueprint/src/lib/components/datasetView/DatasetControls.svelte b/web/blueprint/src/lib/components/datasetView/DatasetControls.svelte
@@ -33,7 +33,7 @@
   $: filters = $datasetViewStore.query.filters;
 </script>
 
-<div class="relative mx-8 my-2 flex items-center justify-between pr-4">
+<div class="relative mx-8 my-2 flex items-center justify-between">
   <div class="flex w-full items-center justify-between gap-x-6 gap-y-2">
     <div class="flex w-full flex-row items-center gap-x-4">
       <div class="flex items-center gap-x-2">

diff --git a/web/blueprint/src/lib/components/datasetView/DatasetLoadingPage.svelte b/web/blueprint/src/lib/components/datasetView/DatasetLoadingPage.svelte
@@ -155,13 +155,14 @@
 
         <div>
           {#if firstRowId != null}
-            <div class="text-xl text-gray-700">Preview</div>
+            <div class="mb-2 text-xl text-gray-700">Preview</div>
             <RowItem
               rowId={firstRowId}
               index={0}
               totalNumRows={$firstRow?.data?.total_num_rows}
               {mediaFields}
               {highlightedFields}
+              datasetViewHeight={320}
             />
           {/if}
         </div>

diff --git a/web/blueprint/src/lib/components/datasetView/DatasetSettingsFields.svelte b/web/blueprint/src/lib/components/datasetView/DatasetSettingsFields.svelte
@@ -135,9 +135,9 @@
   <SkeletonText />
 {:else}
   <div class="flex flex-col gap-y-6">
-    <section class="flex flex-col gap-y-4">
+    <section class="flex flex-col gap-y-2">
       <div class="text-lg text-gray-700">Media fields</div>
-      <div class="text-sm text-gray-500">
+      <div class="mb-2 text-sm text-gray-500">
         Media fields are text fields that are rendered large in the dataset viewer. They are the
         fields on which you can compute signals, embeddings, search, and label.
       </div>
@@ -255,7 +255,7 @@
     </section>
 
     <section class="flex flex-col gap-y-1">
-      <div class="text-lg text-gray-700">View type</div>
+      <div class="mb-2 text-lg text-gray-700">View type</div>
       <div class="flex gap-x-2">
         <Chip
           icon={Table}

diff --git a/web/blueprint/src/lib/components/datasetView/ItemMedia.svelte b/web/blueprint/src/lib/components/datasetView/ItemMedia.svelte
@@ -33,7 +33,6 @@
     type LilacValueNode,
     type Path
   } from '$lilac';
-  import {SkeletonText} from 'carbon-components-svelte';
   import {
     CatalogPublish,
     ChevronDown,
@@ -56,6 +55,7 @@
   // The root path contains the sub-path up to the point of this leaf.
   export let rootPath: Path | undefined = undefined;
   export let isFetching: boolean | undefined = undefined;
+  export let datasetViewHeight: number | undefined = undefined;
 
   let childPathParts: string[];
   // Find all the children path parts that match a media field.
@@ -74,7 +74,7 @@
         if (lastMediaPath == null) continue;
 
         const subPath = [...rootPath, field.path[rootPath.length]];
-        const valueNodes = getValueNodes(row!, subPath);
+        const valueNodes = row != null ? getValueNodes(row, subPath) : [];
         for (const childNode of valueNodes) {
           const childPath = L.path(childNode)![rootPath.length];
           if (childPath != null) {
@@ -115,7 +115,7 @@
     }
   }
 
-  $: valueNodes = row != null ? getValueNodes(row, rootPath!) : [];
+  $: valueNodes = row != null ? getValueNodes(row, rootPath!) : null;
 
   // The child component will communicate this back upwards to this component.
   let textIsOverBudget = false;
@@ -126,7 +126,7 @@
   const datasetViewStore = getDatasetViewContext();
   const appSettings = getSettingsContext();
 
-  $: value = L.value(valueNodes[0]) as string;
+  $: value = valueNodes != null ? (L.value(valueNodes[0]) as string) : null;
 
   $: settings = querySettings($datasetViewStore.namespace, $datasetViewStore.datasetName);
 
@@ -329,23 +329,21 @@
       {/if}
 
       <div class="grow pt-1">
-        {#if isFetching}
-          <SkeletonText class="!w-80" />
-        {:else if value == null || row == null}
-          <span class="ml-12 italic">null</span>
-        {:else if colCompareState == null && spanValuePaths != null && field != null}
+        {#if colCompareState == null && field != null}
           <ItemMediaTextContent
             hidden={markdown}
             text={value}
             {row}
             path={rootPath}
             {field}
             isExpanded={userExpanded}
-            spanPaths={spanValuePaths.spanPaths}
-            spanValueInfos={spanValuePaths.spanValueInfos}
+            spanPaths={spanValuePaths?.spanPaths || []}
+            spanValueInfos={spanValuePaths?.spanValueInfos || []}
             {datasetViewStore}
             embeddings={computedEmbeddings}
             {viewType}
+            {isFetching}
+            {datasetViewHeight}
             bind:textIsOverBudget
           />
           <div class="markdown w-full" class:hidden={!markdown}>
@@ -354,7 +352,14 @@
             </div>
           </div>
         {:else if colCompareState != null}
-          <ItemMediaDiff {row} {colCompareState} bind:textIsOverBudget isExpanded={userExpanded} />
+          <ItemMediaDiff
+            {row}
+            {colCompareState}
+            bind:textIsOverBudget
+            isExpanded={userExpanded}
+            {datasetViewHeight}
+            {isFetching}
+          />
         {/if}
       </div>
     </div>
@@ -387,6 +392,8 @@
           {mediaFields}
           {row}
           {highlightedFields}
+          {datasetViewHeight}
+          {isFetching}
         />
       </div>
     {/each}

diff --git a/web/blueprint/src/lib/components/datasetView/ItemMediaDiff.svelte b/web/blueprint/src/lib/components/datasetView/ItemMediaDiff.svelte
@@ -2,22 +2,29 @@
   import type * as Monaco from 'monaco-editor/esm/vs/editor/editor.api';
   import {onDestroy, onMount} from 'svelte';
 
-  import {getMonaco, MONACO_OPTIONS} from '$lib/monaco';
+  import {
+    DEFAULT_HEIGHT_PEEK_SCROLL_PX,
+    DEFAULT_HEIGHT_PEEK_SINGLE_ITEM_PX,
+    getMonaco,
+    MONACO_OPTIONS
+  } from '$lib/monaco';
   import {getDatasetViewContext, type ColumnComparisonState} from '$lib/stores/datasetViewStore';
   import {getDisplayPath} from '$lib/view_utils';
-  import {getValueNodes, L, type LilacValueNode} from '$lilac';
+  import {getValueNodes, L, type DatasetUISettings, type LilacValueNode} from '$lilac';
   import {PropertyRelationship} from 'carbon-icons-svelte';
   import {hoverTooltip} from '../common/HoverTooltip';
 
   const MAX_MONACO_HEIGHT_COLLAPSED = 360;
-  const MAX_MONACO_HEIGHT_EXPANDED = 720;
 
   const datasetViewStore = getDatasetViewContext();
 
-  export let row: LilacValueNode;
+  export let row: LilacValueNode | undefined | null;
   export let colCompareState: ColumnComparisonState;
   export let textIsOverBudget: boolean;
   export let isExpanded: boolean;
+  export let viewType: DatasetUISettings['view_type'] | undefined = undefined;
+  export let datasetViewHeight: number | undefined = undefined;
+  export let isFetching: boolean | undefined = undefined;
 
   let editorContainer: HTMLElement;
 
@@ -27,8 +34,8 @@
   $: rightPath = colCompareState.swapDirection
     ? colCompareState.column
     : colCompareState.compareToColumn;
-  $: leftValue = L.value(getValueNodes(row, leftPath)[0]) as string;
-  $: rightValue = L.value(getValueNodes(row, rightPath)[0]) as string;
+  $: leftValue = row != null ? (L.value(getValueNodes(row, leftPath)[0]) as string) : '';
+  $: rightValue = row != null ? (L.value(getValueNodes(row, rightPath)[0]) as string) : '';
 
   let monaco: typeof Monaco;
   let editor: Monaco.editor.IStandaloneDiffEditor;
@@ -38,7 +45,10 @@
       relayout();
     }
   }
-
+  $: maxMonacoHeightCollapsed = datasetViewHeight
+    ? datasetViewHeight -
+      (viewType === 'scroll' ? DEFAULT_HEIGHT_PEEK_SCROLL_PX : DEFAULT_HEIGHT_PEEK_SINGLE_ITEM_PX)
+    : MAX_MONACO_HEIGHT_COLLAPSED;
   function relayout() {
     if (
       editor != null &&
@@ -51,10 +61,12 @@
       );
       textIsOverBudget = contentHeight > MAX_MONACO_HEIGHT_COLLAPSED;
 
-      if (isExpanded || !textIsOverBudget) {
-        editorContainer.style.height = `${Math.min(contentHeight, MAX_MONACO_HEIGHT_EXPANDED)}px`;
+      if (isExpanded) {
+        editorContainer.style.height = contentHeight + 'px';
+      } else if (!textIsOverBudget) {
+        editorContainer.style.height = `${Math.min(contentHeight, maxMonacoHeightCollapsed)}px`;
       } else {
-        editorContainer.style.height = MAX_MONACO_HEIGHT_COLLAPSED + 'px';
+        editorContainer.style.height = maxMonacoHeightCollapsed + 'px';
       }
       editor.layout();
     }
@@ -93,7 +105,8 @@
   });
 </script>
 
-<div class="relative -ml-6 flex h-fit w-full flex-col gap-x-4">
+<!-- For reasons unknown to me, the -ml-6 is required to make the autolayout of monaco react. -->
+<div class="relative left-16 -ml-10 flex h-fit w-full flex-col gap-x-4 pr-6">
   <div class="flex flex-row items-center font-mono text-xs font-medium text-neutral-500">
     <div class="ml-8 w-1/2">{getDisplayPath(leftPath)}</div>
     <div class="ml-8 w-1/2">{getDisplayPath(rightPath)}</div>
@@ -106,6 +119,10 @@
     </div>
   </div>
   <div class="editor-container" bind:this={editorContainer} />
+  {#if isFetching}
+    <!-- Transparent overlay when fetching rows. -->
+    <div class="absolute inset-0 flex items-center justify-center bg-white bg-opacity-70" />
+  {/if}
 </div>
 
 <style lang="postcss">