Adds new GoaT NLP UI along with seamless streaming between frontend and servers #33
Conversation
Refined logic for time based queries
…bug_output_html
Improved test cases to suit the new pipeline
Reviewer's Guide by SourceryThis pull request introduces a new GoaT NLP UI with seamless streaming between frontend and servers. The changes include updates to the backend Python code for handling queries and processing data, as well as the addition of a new Next.js-based frontend UI with various components for chat functionality, model selection, and user interactions. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @deepnayak - I've reviewed your changes - here's some feedback:
Overall Comments:
- This PR adds a new GoaT NLP UI with streaming functionality between the frontend and servers. It includes significant updates to the chat interface, new components for handling user input and displaying responses, and integration with the GoaT API.
- The changes introduce a more robust error handling system and improved state management for the chat application. However, it would be beneficial to add more comprehensive documentation for the new components and functions to aid future maintenance and development.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 4 issues found
- 🟡 Documentation: 4 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -29,6 +34,7 @@ | |||
| LlamaIndexInstrumentor().instrument(tracer_provider=tracer_provider) | |||
|
|
|||
| app = Flask("goat_nlp") | |||
| CORS(app) | |||
There was a problem hiding this comment.
🚨 suggestion (security): Specify allowed origins for CORS
For better security, consider specifying allowed origins rather than allowing all origins. This helps prevent unauthorized access from potentially malicious sources.
| CORS(app) | |
| CORS(app, resources={r"/*": {"origins": ["https://yourdomain.com", "http://localhost:3000"]}}) |
| @@ -27,6 +28,7 @@ | |||
| def identify_index(input: str, state: Dict[str, Any]): | |||
| index_response = Settings.llm.complete(INDEX_PROMPT.format(query=input)).text | |||
| state["index"] = json.loads(extract_json_str(index_response)) | |||
| state["status"] = "Identify Index" | |||
There was a problem hiding this comment.
suggestion: Consider using an enum for status values
Using an enum for status values would provide better type safety and prevent potential typos in status strings. It would also make it easier to manage and update the list of possible statuses.
from enum import Enum
class Status(Enum):
IDENTIFY_INDEX = "Identify Index"
state["status"] = Status.IDENTIFY_INDEX
| import { ChatRequestOptions } from "ai"; | ||
| import { v4 as uuidv4 } from "uuid"; | ||
|
|
||
| export interface ChatProps { |
There was a problem hiding this comment.
suggestion: Consider grouping related props to improve maintainability
The ChatProps interface has many properties. Consider grouping related props into sub-objects (e.g., messageProps, inputProps) to improve readability and maintainability. This could make the component easier to use and understand.
export interface ChatProps {
messageProps: {
chatId?: string;
// other message-related props
};
inputProps: {
setSelectedModel: React.Dispatch<React.SetStateAction<string>>;
// other input-related props
};
}
| './src/**/*.{ts,tsx}', | ||
| ], | ||
| prefix: "", | ||
| theme: { |
There was a problem hiding this comment.
suggestion: Optimize color definitions using Tailwind's opacity modifiers
There's repetition in color definitions. Consider using Tailwind's color opacity modifiers (e.g., 'primary/80' for 80% opacity) to reduce repetition and make the config more maintainable.
theme: {
extend: {
colors: {
primary: {
DEFAULT: '#3490dc',
'80': 'rgba(52, 144, 220, 0.8)',
'60': 'rgba(52, 144, 220, 0.6)',
'40': 'rgba(52, 144, 220, 0.4)',
'20': 'rgba(52, 144, 220, 0.2)',
},
},
},
container: {
|
|
||
| return ( | ||
| <div className="relative my-4 overflow-scroll overflow-x-scroll flex flex-col text-start "> | ||
| <Button |
There was a problem hiding this comment.
suggestion: Enhance accessibility of the copy button
Add an aria-label to the copy button to improve accessibility. For example: aria-label="Copy code to clipboard". Also, consider using a more robust solution for managing the copied state, such as a custom hook, instead of setTimeout.
<Button
onClick={copyToClipboard}
variant="ghost"
aria-label="Copy code to clipboard"
| for entity in entities: | ||
| assert ( | ||
| (entity["scientific_name"].lower() in [x.lower() for x in expected_entities]) | ||
| or (entity["singular_form"].lower() in [x.lower() for x in expected_entities]) | ||
| or (entity["plural_form"].lower() in [x.lower() for x in expected_entities]) | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if expected_rank is None: | ||
| pytest.skip("No expected rank for this test case") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if expected_attribute is None: | ||
| pytest.skip("No expected attribute for this test case") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if expected_time_from is None and expected_time_to is None: | ||
| pytest.skip("No expected time for this test case") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if expected_attribute is None or expected_attribute_condition is None: | ||
| pytest.skip("No expected attribute for this test case") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
There was a problem hiding this comment.
Looking good - a couple of thoughts:
- It would be great to have a bit of text with the GoaT link - even with the larger font it is still quite hard to spot. An alternative would be to return the link after the explanation so it stays closer to the text box that the user will be focussed on.
- the models in the ui portion, it seems to need llama2 - is there a way to have this use llama3.1
- Running this locally I'm having trouble asking questions about the returned result - have you pushed that feature yet?
|
Querying the JSON context is working well now I have the models sorted out |
Summary by Sourcery
Add a new GoaT NLP UI with seamless streaming between frontend and servers, enhancing user interaction. Introduce new chat features like voice input, code highlighting, and model switching. Update the installation guide and add comprehensive tests for the query pipeline components.
New Features:
Enhancements:
Build:
Documentation:
Tests: