Feature/rq dashboard #149
Feature/rq dashboard #149
Conversation
albrja
requested review from
beatrixkh,
collijk,
hussain-jafari,
mattkappel,
rmudambi and
stevebachmeier
as code owners
| command, shell=True, stdout=rq_dashboard_log, stderr=rq_dashboard_log | ||
| ) | ||
|
|
||
| atexit.register(proc.kill) |
There was a problem hiding this comment.
Hook to kill dashboard when psimulate runner loop ends.
| logger.info("Fetching redis urls and starting RQ-Dashboard") | ||
| split_urls = " -u ".join(url for url in redis_urls) | ||
| command = "rq-dashboard -u " + split_urls + " --debug" | ||
|
|
There was a problem hiding this comment.
I think this implementation could be improved depending on what we want. Currently, all redis urls are submitted to the dashboard but it doesn't have to be that way. Users could also open up another dashboard with just one redis db. There are a lot of options here.
There was a problem hiding this comment.
We want all redis urls in the same dashboard, we just want to change the way the dashboard handles the information
| split_urls = " -u ".join(url for url in redis_urls) | ||
| command = "rq-dashboard -u " + split_urls + " --debug" | ||
|
|
||
| rq_dashboard_log.write(f"Dashboard running at http://{hostname}:9181\n") |
There was a problem hiding this comment.
This log currently only captures the local url and the url that the user can navigate to for the dashboard. Room for improvement here.
| written_results, | ||
| unwritten_results, | ||
| batch_size, | ||
| output_directory, written_results, unwritten_results, batch_size |
There was a problem hiding this comment.
Did you delete the trailing comma here? That's what induces the whitespace noise.
There was a problem hiding this comment.
I've been having issues with black/isort so I'll add this back in.
| @@ -107,6 +102,26 @@ def try_run_vipin(log_path: Path) -> None: | |||
| logger.warning(f"Performance reporting failed with: {e}") | |||
|
|
|||
|
|
|||
| def run_rq_dashboard(redis_urls: list, output_directory: Path) -> None: | |||
There was a problem hiding this comment.
I'd make a monitoring module and put this function in it. It'll eventually become a subpackage with a bunch of other stuff in it.
There was a problem hiding this comment.
Also, call your output_directory something more descriptive like logging_root.
| hostname = url.split(":")[0] | ||
|
|
||
| # Set up log file | ||
| rq_dashboard_log = (output_directory / "rq.log").open("a") |
There was a problem hiding this comment.
rq.log is a bad name for this. Use rq_dashboard.log.
| split_urls = " -u ".join(url for url in redis_urls) | ||
| command = "rq-dashboard -u " + split_urls + " --debug" | ||
|
|
||
| rq_dashboard_log.write(f"Dashboard running at http://{hostname}:9181\n") |
There was a problem hiding this comment.
This is a lie. YOu haven't launched the dashboard at the time you're writing to this log file.
| command = "rq-dashboard -u " + split_urls + " --debug" | ||
|
|
||
| rq_dashboard_log.write(f"Dashboard running at http://{hostname}:9181\n") | ||
| logger.info(f"Dashboard running at http://{hostname}:9181") |
| # Set up log file | ||
| rq_dashboard_log = (output_directory / "rq.log").open("a") | ||
| logger.info("Fetching redis urls and starting RQ-Dashboard") | ||
| split_urls = " -u ".join(url for url in redis_urls) |
There was a problem hiding this comment.
Two things:
" -u ".join(url for url in redis_urls) is the same as " -u ".join(redis_urls)
and
url_flags = " ".join([f'-u {url}' for url in redis_urls])
means you can write
command = "rq-dashboard " + url_flags + " --debug"
without the fencepost -u in the command.
|
|
||
| # Grab redis urls for dashboard and send them to function for popen | ||
| rq_urls = [ | ||
| f"redis://{hostname}.cluster.ihme.washington.edu:{port}" |
There was a problem hiding this comment.
The workers are okay with this change?
There was a problem hiding this comment.
I'm not sure I understand your question but this isn't doing anything to the workers it is just formatting the redis urls the way the rq-dashboard wants them. It isn't doing anything to the workers themselves to my knowledge.
There was a problem hiding this comment.
So it's NOT* changing the workers by giving them the FQD with* .cluster.ihme.washington.edu. I would presume that so long as the cluster's FQD doesn't change, we're ok.
There was a problem hiding this comment.
@mattkappel Are you saying that f"redis://{hostname}:{port}" resolves to "redis://{hostname}.cluster.ihme.washington.edu:{port}"? If so, then no need for an rq_urls and you can just pass in redis_urls?
There was a problem hiding this comment.
oh sorry, I misread! rq_urls != redis_urls But I would think what you think I said is true.
…and for future use
|
One important note is I had to make a few changes to the cli.py file in the rq-dashboard package since the package itself has not been updated in a few years but click has so to get this working I had to make my own branch to fix some default settings for click options which is not reflected in this PR and the RQ-Dashboard as implemented here will not work without making these changes in the rq-dashboard package. |
| command = "rq-dashboard " + url_flags + " --debug" | ||
|
|
||
| proc = subprocess.Popen( | ||
| command, shell=True, stdout=rq_dashboard_log, stderr=rq_dashboard_log |
There was a problem hiding this comment.
Do you want stdout and stderr going to the same log?
There was a problem hiding this comment.
I mean, it's sometimes useful to have an log.o and and log.e but generally I don't think that's useful for our logging purposes in psimulate.
| command, shell=True, stdout=rq_dashboard_log, stderr=rq_dashboard_log | ||
| ) | ||
| rq_dashboard_log.write(f"Dashboard running at http://{hostname}:9181\n") | ||
| logger.info(f"Dashboard running at http://{hostname}:9181") |
There was a problem hiding this comment.
Where did these static ports come from?
There was a problem hiding this comment.
In the vivarium_dashboard package the cli.py that has all the click options sets 9181 as the default.
There was a problem hiding this comment.
We ought to have psimulate pick a random open port like @collijk suggested and pass that in as an option.
| written_results, | ||
| unwritten_results, | ||
| batch_size=batch_size, | ||
| output_directory, written_results, unwritten_results, batch_size=batch_size |
There was a problem hiding this comment.
similar to above, i think black will add in a trailing comma and the line breaks
Feature/RQ-Dashboard
Description
Added functionality to run a RQ-dashboard when user launches a parallel simulation.
-Added run_rq_dashboard function that launches dashboard during main psimulate runner loop
-Dashboard captures all jobs submitted on that host for all redis databases that were provide when dashboard was initialized (current implementation is all redis databases but this could be changed with little work).
-Hook to kill subprocess running dashboard when the parallel simulation ends.
-Added rq.log to log subprocess and provide user with URL to navigate to use the dashboard. URL is also printed to terminal.
Testing
Ran multiple parallel simulation and successfully saw working implementation for:
-Creation and usage of new rq.log file
-Dashboard launches at start of simulation and the process ends when the simulation finishes
-All redis databases used for parallel simulation are attached to the dashboard depending on how many were used for that simulation.