- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3
Clarifycoder running on DRA using Okanagan Framework #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| test_case_list.append(test_case) | ||
|  | ||
| # Use Gemini if GEMINI_API_KEY is available, otherwise fallback to OpenAI | ||
| use_gemini = os.getenv("GEMINI_API_KEY") and os.getenv("GEMINI_API_KEY") != "" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use gemini by default? this might break this code and existing runs that relies on openai API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not put a Gemini key, by default the code will revert back to using Openai API by default this is a check done to use gemini when GEMINI_API_KEY is available as environment variable
| call_demo*.py | ||
| demo*.py | ||
| __pycache__/* | ||
| __pycache__ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removing /* here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done to ignore pycache dirs completely not keep the folder there and just ignore whats inside
| import concurrent.futures | ||
| import google.generativeai as genai | ||
| import os | ||
| import time | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this imported twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time being imported twice is just a mistake
| completions_code.append(completion) | ||
|  | ||
| # Use Gemini if GEMINI_API_KEY is available, otherwise fallback to OpenAI | ||
| use_gemini = os.getenv("GEMINI_API_KEY") and os.getenv("GEMINI_API_KEY") != "" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repetitive code here, please abstract this into a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the repetitive nature of it but I feel like it is a bit more visual to keep it that way. But I will abstract into function
| client = openai.OpenAI() | ||
| completion = client.chat.completions.create( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested this openai usage? why is this change necessary? have you updated the README regarding installation?
| # Return the first triple code snippet. | ||
| def response_2_code(response): | ||
| code_template = re.compile('```.*\n([\s\S]+?)\n```', re.M) | ||
| code_template = re.compile(r'```.*\n([\s\S]+?)\n```', re.M) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for
| return -1 | ||
| # Use Gemini if OpenAI keys are dummy/empty, otherwise use OpenAI | ||
| openai_key = os.environ.get('OPENAI_API_KEY', '') | ||
| use_gemini = openai_key == "dummy" or openai_key == "" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for this file. the code base is becoming very complicated with your gemini change. Please refactor this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please abstract our the use gemini or openai logic as mentioned, otherwise the codebase is much more complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file has many previous runs. Please be very careful about any change in this class. You should not let gemini override openai. Please pass an argument for using gemini for your use case and this code should use openai by default for maintaining previous runs.
| please test this code and add screenshots of the results. Also add the commands you use to run the code in this PR and in README. | 
| please add PR description, currently it's empty | 
No description provided.