-
Notifications
You must be signed in to change notification settings - Fork 68
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
GUI panel for mlos_bench #824
base: main
Are you sure you want to change the base?
Conversation
- [MLOS Viz Panel](#mlos-viz-panel) | ||
- [Usage](#usage) | ||
- [Running the Backend](#running-the-backend) | ||
- [Running the Frontend](#running-the-frontend) |
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.
There's no content for these entries it looks like. Is that intentional?
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.
Forget to commit something?
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.
@bpkroth Check internal repo might be there
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.
@yshady this is effectively a new effort, so I'm not necessarily looking to replicate that.
@@ -0,0 +1,53 @@ | |||
|
|||
|
|||
### MLOS Viz Panel |
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.
FYI, this doesn't pass a markdownlint check.
@@ -0,0 +1,124 @@ | |||
from datetime import datetime, timedelta |
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.
Needs some linting/style/formatting/etc.
@@ -0,0 +1,124 @@ | |||
from datetime import datetime, timedelta |
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.
Module docstring and/or some README.md with details on what this is
a) supposed to do,
b) how it does it,
subscription_id = global_config['subscription'] | ||
|
||
# Load the storage config and connect to the storage | ||
storage_config_path = "config/storage/mlos-mysql-db.jsonc" |
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.
Should probably be configurable.
return count_str | ||
|
||
# Load credentials from the JSON file | ||
with open('azure_openai_credentials.json', 'r') as file: |
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.
Should be configurable
|
||
if __name__ == "__main__": | ||
import uvicorn | ||
uvicorn.run(app, host="0.0.0.0", port=8000, reload=True) |
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.
Some discussion in the README.md about networking requirements for this should also happen.
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.
Also instructions for deploying this in the cloud would be helpful.
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.
Yeah I agree but there are some restrictions within the team on deployment so never got to that
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.
Totally fair, but this doesn't have those same constraints. It does have additional goals though.
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.
Agreed also will need more reliability and testing of the systems before it is feasible, maybe more long term
warnings.filterwarnings("ignore", category=FutureWarning) | ||
|
||
# Ensure the backend is running on this port | ||
backend_url = "http://localhost:8000" |
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 should be configurable to happen on a different host.
|
||
|
||
# Function to plot correlation between parameter changes and latency | ||
def plot_param_latency_correlation(experiment_id, storage, metric): |
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.
Think most of these plot_*
methods could probably be split out to a separate set of modules for readability.
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.
A lot of them aren’t used, heads up
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.
Remove or comment them out for now then?
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.
Might also argue that this is intended to do more than just visualization, correct?
It's also intended to be able to launch new experiments from existing config dirs, so we could call it "mlos_webui" or some such and will probably want to make it pip installable in that case.
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.
Correct my intention was to launch this so multiple people could access the same set of experiments easily. Example me and @eujing could theoretically be collaborating on the same set of experiments, both monitoring and making sure everything is smooth sailing.
Execution isn’t there but was worth a shot. Was constrained by time and lack of testing really.
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.
My goal was basically to turn Mlos into a web app that can be deployed with a login page :)
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.
Even benchmarks should be configurable from a GUI that would be a pretty web app in my opinion
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.
Important to note though that this pr removes launching experiment functionality and solely focuses on visualizations, again check internal repo as it is far more comprehensive
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.
Let's focus on doing this externally for now.
And I understand the original constraints, but we don't have those now, so we can be a little more methodical about what and how we want to design parts of that.
I'm not opposed to either launching or config editing at a high level, though I have opinions about the implementation details and constraints around those, so let's start with a list of needs, wants, would be nices and then chart a course for us to get there.
I'll start a new Issue to track some of that.
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.
My personal experience is that ui work often requires manual user testing and no units tests / checks will be sufficient, may work for backend work not so much for frontend user experience stuff especially |
Web pages can be tested too: |
Definitely possible but most bug fixes will probably come from user bug reports but again my personal experience might not reflect the future |
No description provided.