Frevagpt #69

BiancaWentzel · 2024-12-12T15:09:56Z

@Karinon Could you have a look at the whole frevagpt and suggest changes and improvements?
I don't really want to merge the changes to main as long as the feature is still in development but a genereal look at the codebase would be helpful.

Thank you

Data treaming

antarcticrainforest · 2024-12-12T16:22:50Z

We need to hammer out a proper deployment strategy. Currently, I am quite doubtful that we have a mild plan for shipping the whole thing. I haven't even seen a PR/MR for the backend.

Karinon

This is part one. I will comment more stuff later.

Karinon · 2024-12-13T08:20:27Z

webpack/webpack.base.config.js

+            resolve: {
+                fullySpecified: false
+            }
+        },


Is this also an old fragment? I couldn't find a purpose.

Seems to be the case

Karinon · 2024-12-13T08:26:05Z

webpack.config.js

+          buffer: require.resolve("buffer"),
+        },
+      },
+    });


None of the changes are necessary anymore except for maybe Dotenv (which could be moved to webpack.base.config.js). Even this one would not be necessary when you apply all the changes I recommend, but I guess you can leave it here, as I am sure it will come in handy another time (I had already some ideas)..

Karinon · 2024-12-13T08:28:55Z

package.json

    "react-table": "^7.7.0",
    "react-window": "^1.8.8",
    "redux": "^4.2.1",
    "redux-logger": "^3.0.6",
    "redux-thunk": "^2.4.1",
-    "react-select": "5.7.3"
+    "remark-gfm": "^3.0.1",
+    "stream-browserify": "^3.0.0"


stream-browserify, JSONStream and buffer are no longer necessary.

I would also propose to remove bootstrap-icons. For this purpose we already have react-icons (https://react-icons.github.io/react-icons/) which you should use instead. Consider to use the FontAwesome Set there, as this is what I was using for everything else (with a few minor exceptions) to keep things consistent.

Karinon · 2024-12-13T08:42:53Z

django_evaluation/settings/local.py

@@ -281,6 +298,12 @@ def _set_favicon(html_color: str, project_root: Path) -> None:
    "variable",
 ]
 MENU_ENTRIES = []
+CHAT_BOT_URL = "http://vader5-icpub.lvt.dkrz.de:8502"


This should be configurable via os.environ.get. @antarcticrainforest is this how you want this? I lost track when we are using web_config.get and when os.environ.get

django_evaluation/views.py

django_evaluation/urls.py

Karinon · 2024-12-13T08:49:36Z

django_evaluation/settings/local.py

@@ -77,6 +77,23 @@ def _get_logo(logo_file, project_root):
    return f"/static/img/{logo_file.name}"


+def _read_secret(port: int = 5002, key: str = "email") -> dict[str, str]:


@antarcticrainforest I have no clue about those changes and where they come from. Do we need this? Apparently it is not in use.

Karinon · 2024-12-13T08:51:59Z

assets/js/Containers/FrevaGPT/utils.js

+  return codeSnippets;
+}
+
+export const objectToQueryString = (obj) => {


We have a dependency query-string which does essentially the same thing. Use that instead

assets/js/Containers/FrevaGPT/components/PendingAnswerComponent.js

bot/proxyviews.py

Karinon · 2024-12-13T10:23:29Z

assets/js/Containers/FrevaGPT/index.js

+        const response = await fetch("/api/chatbot/ping");
+        if (response.status === 200) pingSuccessful = true;
+      } catch (err) {
+        console.error("PingError: ", err);


console.error is fine for debugging, but we don't want it in the finished code

Karinon · 2024-12-13T10:27:07Z

assets/js/Containers/FrevaGPT/index.js

+  async fetchData(input) {
+    const queryObject = {
+      input,
+      auth_key: process.env.BOT_AUTH_KEY,


Having process.env.BOT_AUTH_KEY in the react-code means that we are exposing our API-Key to the whole world, which we really don't want. This must be moved to proxyview.py instead. The same is true for freva_config.

assets/js/Containers/FrevaGPT/index.js

Karinon · 2024-12-13T13:44:11Z

assets/js/Containers/FrevaGPT/components/SidePanel.js

+  return (
+    <div>
+      <Card className="mb-3 shadow-sm">
+        <div className="btn btn-outline-secondary border-0 p-3 rounded-top text-start card-header shadow-sm button-div">


The outer div doesn't seem necessary, you can replace it by a simple <>

Don't use btn here. Due to the btn the user experience here implies that the Header of the section/Card is clickable, which it isn't. A simple <Card.Header> is fine as well

Likewise, <Card.Body> looks fine for me, too. It isn't collapsible.

Maybe add some vertical margins between the links. It is visually a bit hard to distinguish between the different links.

What's the point of hiding the link to the thread behind a onClick? I think a <a className="text-wrap" href={`/chatbot/?thread_id=${element.thread}`} > would be totally fine. It would be even better, if you could use <Link to... instead (from import { Link } from "react-router";) because it wouldn't cause the whole website to reload and the user would stay inside react, but this would need more adjustments.

Karinon · 2024-12-13T13:59:54Z

assets/js/Containers/FrevaGPT/reducers.js

+export const frevaGPTReducer = (state = frevaGPTInitialState, action) => {
+  switch (action.type) {
+    case constants.SET_CONVERSATION:
+      return { ...state, conversation: action.payload };


Retrieving old threads leads to unescaped URI-Fragments in the User-Variants of the thread, which looks quite ugly (Name%20the%20most%20important%20parameters%20in%20meteorology).

I would blame the backend for this, but I am not entirely sure as I have never seen the backend. We could try to solve it here, e.g. with something like this:

case constants.SET_CONVERSATION: { const payload = action.payload; for (const payloadElem of payload) { if (payloadElem.variant === "User") { payloadElem.content = decodeURIComponent(payloadElem.content); } } return { ...state, conversation: payload }; }

Take it with a grain of salt, as I don't have a complete overview over possible edge-cases

assets/js/Containers/FrevaGPT/index.js

Karinon · 2024-12-13T14:27:33Z

assets/js/Containers/FrevaGPT/index.js

+    this.setState({ userInput: "" });
+  }
+
+  async handleSubmit(input) {


It would be reasonable to prevent empty strings to be passed to the backend

Adapted the affected functions. But edge cases like submitting a whitespace or similar absurd inputs are not covered. Should we thin kabout a more comprehensive approach? The Bot seems to handle this kind of inputs well (at least the one I tested).

My take on this would be something like this as the first action in handleSubmit (since this is the function which performs the actual action either way)

if (!input.trim()) { return; }

Or instead of negating the value (empty strings are falsy) you could as well surround it with isEmpty. By this we would at least cover whitespaces.

Karinon · 2024-12-17T07:25:11Z

webpack.config.js

@@ -3,13 +3,22 @@ const { merge } = require("webpack-merge");
 const baseConfig = require("./webpack/webpack.base.config");
 const devConfig = require("./webpack/webpack.dev.config");
 const prodConfig = require("./webpack/webpack.prod.config");
+const webpack = require('webpack');
+
+const Dotenv = require("dotenv-webpack");


since you moved dotenv to the base, we can delete it here.

Karinon · 2024-12-17T08:01:17Z

django_evaluation/settings/local.py

@@ -282,6 +299,15 @@ def _set_favicon(html_color: str, project_root: Path) -> None:
 ]
 MENU_ENTRIES = []

+CHAT_BOT_URL = "http://vader5-icpub.lvt.dkrz.de:8502"
+CHAT_BOT_AUTH_KEY = os.environ.get("CHAT_BOT_AUTH_KEY")
+CHAT_BOT_FREVA_CONFIG = os.environ.get("CHAT_BOT_AUTH_KEY")


Multiple things here:

You renamed BOT_AUTH_KEY to CHAT_BOT_AUTH_KEY. This is not an issue per se but I stumbled upon this because I was not aware of the change and I am not sure if this was by accident. We can keep it though, I don't mind.

Copy-Paste error in the env-var of CHAT_BOT_FREVA_CONFIG, but:

I assume that CHAT_BOT_FREVA_CONFIG is not needed, as the web has already a env-var for this: EVALUATION_SYSTEM_CONFIG_FILE (I should have mentioned it earlier, but I forgot, sorry). However, it isn't stored in a variable, as far as I can see. So you can rename the var CHAT_BOT_FREVA_CONFIG to EVALUATION_SYSTEM_CONFIG_FILE, I guess. One thing to node is, however, that in dev-envs EVALUATION_SYSTEM_CONFIG_FILE will obviously refer to a file on the local file system where the (currently) globally deployed chatbot has no access to. I have also no idea what the bot does with this information. I have briefly tested it and it seems that it still works

Karinon · 2024-12-17T08:05:13Z

django_evaluation/views.py

@@ -0,0 +1 @@
+


Delete the file

Karinon · 2024-12-17T08:18:37Z

assets/js/Containers/FrevaGPT/index.js

+                  placeholder="Ask a question"
+                />
+                {this.state.loading ? (
+                  <Button variant="outline-danger" onClick={this.handleStop}>


Super nitpicky, but: Could you add className="d-flex align-items-center" to both buttons which contain the icon?

The icons are not vertically centered out of the box and with this classes they look so much nicer.

Karinon · 2024-12-17T09:06:25Z

assets/js/Containers/FrevaGPT/index.js

+      return pingSuccessful;
+    };
+
+    const getBotModels = async () => {


Add error-handling. Do it by checking either the status code or response.ok.

Karinon · 2024-12-17T09:11:21Z

bot/proxyviews.py

+            upstream_response = requests.get(base_url[:-1], params=params, stream=True)
+            upstream_response.raise_for_status()
+        except requests.RequestException as e:
+            return StreamingHttpResponse(


While I was dealing with broken BOT_AUTH_KEYs, I had the pleasure to meet this Exception here and I think we need a few adjustments:

replace 502 by the actual status_code. It seems that it is stored in e.response.status_code

Maybe we also don't want to expose the actual error message, because it contains the URL to the bot-server which we don't want to expose (and probably it is not even available from the outside). We should log the actual error message but we only expose something generic. I have something like this in mind:

logger = logging.getLogger("freva-web") logger.exception(e) status_code = e.response.status_code error_message = HTTPStatus(status_code).phrase

HTTPStatus comes from from http import HTTPStatus

I think it would make things easier if we would deal with a JSON instead of a string. My initial idea would be to replace the error response by something like

return StreamingHttpResponse( json.dumps( { "variant": "ServerError", "content": f"Error while connecting to the external API: {error_message}", } ), content_type="application/json", status=status_code, )

But I am not sure. In other parts of the django backend we simply create errors like {"error": "error message"}, but if it helps you to keep the structure of the rest, feel free to alter it. Keep in mind that this error message will also appear for other bot-requests like availablebots

Karinon · 2024-12-17T09:16:43Z

assets/js/Containers/FrevaGPT/index.js

+    };
+
+    // response of a new bot request is streamed
+    const response = await fetch(


When having an error case (backend returns some kind of Server Error), the user will not see any output. I think before going into reading the stream, you have to check response.ok and respond accordingly.

… input

Karinon and others added 30 commits August 13, 2024 14:37

Boilerplate code for a chat-endpoint in react

75669d5

fixed some sorting issues for the linter...

99fa7e7

Merge branch 'main' of github.com:FREVA-CLINT/freva-web into frevagpt

30a9b54

Catch vault connection errors.

fade7ec

Add skeleton for bot.

e1a0c84

Adapted bot menu entry

f092315

Added conversation functionality including codeblocks and images

604cf5c

Enabled environment variable handling, added dependencies

5708ef5

Changed method of parsing and fetching data stream

476e92f

Updated webpack production configs

76b0665

Added keydown event to input to trigger bot

76415e1

Adapted to utilize Bootstrap

fd37b6c

Included thread id into conversation and fetching

d5d95ba

Adapted styling of codeblock and conversation bubbles

3b6f788

Added sidebar

bf5d0fe

Made Codeblock collapsible

85232c6

Added variant logs

ee64840

Added thread si to url

487b9f7

Made thread id a ref instead of state varaible

ae4c3d1

merged current status of frevagpt

aaaf4c1

Outsourced sidepanel

96eeeae

WIP viewing old threads

2cb1965

adapted conversation to support thread history, adapted codeblock

368fa27

undone conversation changes

8a376f3

added example request on climate data

89890a2

Formatted Code String

b654c8f

Changed data streaming to include buffer being parsed bit by bit

be0ec6b

Adapted fetching of old thread

0057b1b

added example requests

cdb9556

Added hanling of markdown in response

5633336

Merge pull request #68 from FREVA-CLINT/data-treaming

c153a90

Data treaming

BiancaWentzel requested a review from Karinon December 12, 2024 15:09

Karinon reviewed Dec 13, 2024

View reviewed changes

Bianca Wentzel added 14 commits December 16, 2024 15:18

Removed unused fragments

33179fe

Added linting rule preventing one-line-statements without braces

c80eb18

Fixed linting

6ac470c

Exchanged console-error-statements with proper error handling

60d50b7

Exchanged third-party has with built-in hasOwn

4903cfe

Fixed requesting bot on clickung play button

0318363

Submit request to backend only if userInput not empty

217eca6

Removed unused depedencies, including bootstrap icons

7be1cbc

Removed unused plugins, moved Dotenv to base config

f77876f

Exchanged old icons with react icons

f890cf5

Removed unused rule

a2e1486

Moved auth key and freva config to local environment used by proxyview

1ea7b93

Using query-string instead of custom function

0e5c3fd

Adapted SidePanel

81c3f3f

Karinon reviewed Dec 17, 2024

View reviewed changes

Bianca Wentzel and others added 9 commits January 15, 2025 15:39

Removed unused dependencies from webpack config

617f010

Deleted empty file

b5e28fb

Added styling classes to input buttons

8796deb

Trimmed unnecessary spaces from input, preventing submission of empty…

69aaa4b

… input

Added error handling to bot model request

69a1486

Added error handling and message display on server error

465af02

Fixed typo

73b9b1a

Added timeout

1b6bfe3

Merge remote-tracking branch 'origin/frevagpt' into frevagpt

9ea6059

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frevagpt #69

Frevagpt #69

BiancaWentzel commented Dec 12, 2024

antarcticrainforest commented Dec 12, 2024

Karinon left a comment

Karinon Dec 13, 2024

BiancaWentzel Dec 16, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024

Karinon Dec 13, 2024 •

edited

Loading

Karinon Dec 13, 2024

BiancaWentzel Dec 16, 2024

Karinon Dec 16, 2024

Karinon Dec 17, 2024

Karinon Dec 17, 2024

Karinon Dec 17, 2024

Karinon Dec 17, 2024

Karinon Dec 17, 2024

Karinon Dec 17, 2024 •

edited

Loading

Karinon Dec 17, 2024

		@@ -77,6 +77,23 @@ def _get_logo(logo_file, project_root):
		return f"/static/img/{logo_file.name}"


		def _read_secret(port: int = 5002, key: str = "email") -> dict[str, str]:

Frevagpt #69

Are you sure you want to change the base?

Frevagpt #69

Conversation

BiancaWentzel commented Dec 12, 2024

antarcticrainforest commented Dec 12, 2024

Karinon left a 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

Karinon Dec 13, 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

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

Karinon Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karinon Dec 13, 2024 •

edited

Loading

Karinon Dec 17, 2024 •

edited

Loading