-
Notifications
You must be signed in to change notification settings - Fork 0
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
Generalize dispatcher for all job types #81
Conversation
@@ -0,0 +1,9 @@ | |||
from metriq_gym.benchmarks.benchmark import Benchmark |
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.
Instead of three separate imports from the same place, can we just have one that imports all three?
from metriq_gym.benchmarks.benchmark import Benchmark | |
from metriq_gym.benchmarks.benchmark import ( | |
Benchmark, | |
CLOPS, | |
QuantumVolume, | |
) |
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.
Right now they are coming from three different modules, not from the same place. To achieve what you are suggesting, we would need to export them using __init__.py
files, which I find it becomes unmanageable after a while.
logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(levelname)s - %(message)s") | ||
|
||
|
||
def main(): |
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.
Do we need this in a main
block? Can we take this out like so:
def main(): | |
load_dotenv() | |
args = parse_arguments() | |
params = load_and_validate(args.input_file) | |
job_manager = JobManager(args.jobs_file) | |
HANDLERS[JobType(params["benchmark_name"])](args, params, job_manager).dispatch_handler() |
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 like to wrap it inside if __name__ == "__main__":
so it's clear that it's a script meant to be run directly and not imported from other modules.
Note that this is going away as well at some point, and we will have only one cli script with a cli manager handling all the cli commands (Issue #56)
from metriq_gym.job_manager import JobManager | ||
|
||
|
||
class Benchmark: |
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.
Can we make this an abstract class? This would be a nice way to enforce that any subsequent benchmarks we implement have the dispatch and poll handlers.
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.
Do you mean a formal interface defined using abc.ABCMeta
? It feels a bit over-engineering at the moment, given how small the codebase is. Also, until we migrate poll methods too, I wouldn't enforce the handler definition to be too strict.
Description
Generalizing the job dispatcher so that we don't need to create one for each job type.
Code specific to a job type should be created in the handlers.
Note: I will do the same for polling, but in a separate PR to make the PRs easier to digest.
Issue ticket number and link
Fixes #65
Type of change
How Has This Been Tested?
Unit tests, plus smoke tested with
Checklist: