Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

yshady
Copy link

@yshady yshady commented Jul 26, 2024

No description provided.

@yshady yshady requested a review from a team as a code owner July 26, 2024 22:10
- [MLOS Viz Panel](#mlos-viz-panel)
- [Usage](#usage)
- [Running the Backend](#running-the-backend)
- [Running the Frontend](#running-the-frontend)
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget to commit something?

Copy link
Author

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

Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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"
Copy link
Contributor

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:
Copy link
Contributor

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)
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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.

Copy link
Author

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"
Copy link
Contributor

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):
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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?

@yshady
Copy link
Author

yshady commented Aug 8, 2024

@bpkroth #821 Passes all checks

@bpkroth
Copy link
Contributor

bpkroth commented Aug 8, 2024

@bpkroth #821 Passes all checks

There's no checks or tests for this new code, so that's not really very helpful yet.

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Author

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 :)

Copy link
Author

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

Copy link
Author

@yshady yshady Aug 11, 2024

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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpkroth bpkroth mentioned this pull request Aug 12, 2024
9 tasks
@yshady
Copy link
Author

yshady commented Aug 12, 2024

@bpkroth #821 Passes all checks

There's no checks or tests for this new code, so that's not really very helpful yet.

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

@bpkroth
Copy link
Contributor

bpkroth commented Aug 13, 2024

@bpkroth #821 Passes all checks

There's no checks or tests for this new code, so that's not really very helpful yet.

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:
https://www.selenium.dev/documentation/webdriver/

@yshady
Copy link
Author

yshady commented Aug 13, 2024

@bpkroth #821 Passes all checks

There's no checks or tests for this new code, so that's not really very helpful yet.

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: https://www.selenium.dev/documentation/webdriver/

Definitely possible but most bug fixes will probably come from user bug reports but again my personal experience might not reflect the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants